Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Nutriment Nutrient Units #871

Open
davidpryor opened this issue Jan 29, 2024 · 7 comments
Open

Support Nutriment Nutrient Units #871

davidpryor opened this issue Jan 29, 2024 · 7 comments

Comments

@davidpryor
Copy link
Contributor

Why - Problem description

The openfoodfacts server supports passing of nutriment units through the following parameter "nutriement__unit". The current version of the SDK does not support passing this parameter to the server. Currently, with this restriction, a user must read through the SDK code and/or server code to find what units the backend is expected nutrients to be passed as.

What - Proposed solution

Expand the current Nutriments.setValue method signature to accept an optional (for backwards compatibility), parameter called "unit" of type Unit?. If a unit is passed, save these in the object state similar to how nutriment values are saved. Otherwise, fall back to the currently existing "typicalUnit" attribute on the Nutrient Enum. This fallback isn't technically needed, but may make it easier to for users reading through the code. An alternate/addition would be to update the Nutrient Enum docstrings with their default unit.

The easiest way to add it in, would be to make another internal map state in the Nutriment class called "_unitMap".

class Nutriments extends JsonObject {
  ...
  final Map<String, String> _unitMap = <String, String>{};
  ...
  /// Returns the map key for that [nutrient] unit.
  String _getUnitTag(final Nutrient nutrient) => '${nutrient.offTag}_unit';
  ...
    Nutriments setValue(
    final Nutrient nutrient,
    final PerSize perSize,
    final double? value, {
    final Unit? unit = null,
  }) {
    final Unit finalUnit = unit ?? nutrient.typicalUnit;
    _map[_getTag(nutrient, perSize)] = value;
    _unitMap[_getUnitTag(nutrient)] = finalUnit.offValue;
    return this;
  }
  @override
  Map<String, dynamic> toJson() {
    final Map<String, dynamic> result = <String, dynamic>{};
    for (final Nutrient nutrient in Nutrient.values) {
      for (final PerSize perSize in PerSize.values) {
        final String tag = _getTag(nutrient, perSize);
        final double? value = _map[tag];
        if (value != null) {
          result[tag] = value;
        } else if (_map.containsKey(tag)) {
          result[tag] = '';
        }
        final String? unit = _unitMap[_getUnitTag(nutrient)];
        if (unit != null) {
          result[_getUnitTag(nutrient)] = unit;
        }
      }
    }
    return result;
  }

}

Also, implementing similar enum patterns for the Unit enum, one could move the OFF string representation for each unit onto the enum itself and simplify the serialization.

- enum Unit { KCAL, KJ, G, MILLI_G, MICRO_G, MILLI_L, L, PERCENT, UNKNOWN }
+ enum Unit {
+   KCAL(offValue: 'kcal'),
+   KJ(offValue: 'kj'),
+   G(offValue: 'g'),
+   MILLI_G(offValue: 'mg'),
+   MICRO_G(offValue: 'mcg'),
+   MILLI_L(offValue: 'ml'),
+   L(offValue: 'liter'),
+   PERCENT(offValue: 'percent'),
+   UNKNOWN(offValue: '');
+ 
+   final String offValue;
+   const Unit({required String this.offValue});
+ }

https://github.com/davidpryor/openfoodfacts-dart/tree/expose-nutrient-unit-parameter

Alternatives you've considered

  1. Require users to determine the default unit required for the off-server and convert their internal representation units to the expected unit. This still needs documentation updates to let the user know what units are needed. Also, it more makes sense to expose the server-api through the client.
  2. Same as the solution above but with a large refactor. One moves responsibilities of serializing an individual nutrient/unit/modifier/etc combo into a new class.
@monsieurtanuki
Copy link
Contributor

Hi @davidpryor!

The openfoodfacts server supports passing of nutriment units through the following parameter "nutriement__unit".

I'm not sure it does. I think it is read-only.

The current version of the SDK does not support passing this parameter to the server.

That's correct.

Currently, with this restriction, a user must read through the SDK code and/or server code to find what units the backend is expected nutrients to be passed as.

It's always grams for weights.

  /// Returns the value in grams of that [nutrient] for that [perSize].
  ///
  /// It won't be grams for very specific nutrients, like [Nutrient.alcohol].
  double? getValue(final Nutrient nutrient, final PerSize perSize)  // ...

  /// Sets the value in grams of that [nutrient] for that [perSize].
  ///
  /// It won't be grams for very specific nutrients, like [Nutrient.alcohol].
  Nutriments setValue(
    final Nutrient nutrient,
    final PerSize perSize,
    final double? value,
  ) // ...

Please share with us a new unit test that fails, according to you.

@davidpryor
Copy link
Contributor Author

davidpryor commented Jan 30, 2024

Hey @monsieurtanuki thank you for your response!
Let me know your thoughts.

I'm not sure it does. I think it is read-only.

Through my testing with my implementation, it seems this is a writable field:

On my new food, I tested setting carbs to 17000 mg

"carbohydrates_100g" -> 17000.0
"carbohydrates_unit" -> "mg"

The parameter map generates

"nutriment_carbohydrates" -> "17000.0"
"nutriment_carbohydrates_unit" -> "mg"

and the GET for the product returns

"carbohydrates_100g" -> 17.0

It's always grams for weights.

But the doc string also says /// It won't be grams for very specific nutrients, like [Nutrient.alcohol]. which leaves the user wondering "which specific nutrients?"

I originally looked to the Nutrient enum itself since they carry the "typicalUnit" attribute, which I learned was incorrect 😁

Going back to my proposal, the fallback unit could be grams for all but the special cases. This way to ensure backwards compatibility.

Please share with us a new unit test that fails, according to you.

I can provide a working test, but not failing since the feature doesn't exist yet 😁

davidpryor@6c18c71

@monsieurtanuki
Copy link
Contributor

@davidpryor If I remember well there was limited added value in also saving the unit, and it was an additional source of confusion.
Currently we're able to save any nutrient value. Your unit ('mg') is not saved on the server, it's just used for a conversion on the server just before saving the data.
Maybe you're ok with "I need to say it's 17000 mg, and then the server says it's 17 (implicitly) grams and does not store that I prefer a display in mg instead of g"
We preferred to say "Just put the value in grams, for that you'll just have to learn how to divide by 1000 or 1000000, and we don't care about your unit".

which leaves the user wondering "which specific nutrients?"

My guess: nutrients which are not in g, mg and µg.

I originally looked to the Nutrient enum itself since they carry the "typicalUnit" attribute, which I learned was incorrect

I didn't know that. Please create a PR to correct the wrong "typicalUnit"s.

@davidpryor
Copy link
Contributor Author

Thanks for the response @monsieurtanuki !
I think we may be talking about two different points now.
What I am requesting is to expose the write API for sending a nutrient's unit. This will allow the server to do the auto conversion to grams.

@monsieurtanuki
Copy link
Contributor

What I am requesting is to expose the write API for sending a nutrient's unit. This will allow the server to do the auto conversion to grams.

That's what I've understood.

The problem is that as I've just written before, we decided NOT to let the user set a unit because:

  • the normal expectation would be to get the same unit (e.g. mg) when reading the unit after writing it - and it's not the case as you've noticed in your "17000mg => 17g" test.
  • most developers know how to divide by 1000 or 1000000, and they'll have to multiply later anyway "17g => 17000mg"
  • the confusion is not worth it
    • "hey, I said it's 17000mg, why do you say it's 17g instead?"
    • "hey, I have the right to set the unit of proteins in kcal"
    • and so on

Therefore unless you have a good reason - e.g. you're blocked in your app - I don't really see the point of adding confusion to the package. Or maybe there's something that I don't understand. Again.

@CharlesNepote
Copy link
Member

CharlesNepote commented Jun 24, 2024

Just a few comments.

I'm not able to understand the technical impacts, but what I can say is that for Open Food Facts, units do really matters.

One of the very common rule we use to tell the contributors is: enter the data as displayed on the packaging (with the same units):

  1. This leads users (I mean readers) to trust the data if they find the same value/units as displayed on the packaging.
  2. It eases the work for data fixers: a team of ~40 is receiving a daily mission to fix 3 products: it is far easier to fix issues when data are exactly the same as displayed on the packaging
  3. Sometimes you can't enter data in grams (IU, ° for alcohol, etc.).
  4. Also 0 mcg is a bit different than 0 grams: with 0 grams you can have error margin, and it might be 0.01 grams rounded to 0. On the other hand, with 0 mcg, even if there is an error margin, it should not be 10 mcg!

Some countries have standardized the units for nutrients on food packagings, such as the USA, and they do use grams, mg and even mcg.

This is a screenshot of the website when fixing a product. Here you can see that having all values in grams is quite harder to fix. I need to be very careful: is 0.0004 g is corresponding to 0.4 m? Sure I can do the computation mentally, but when fixing dozens of products it's ruining my brain ;-)
image

@monsieurtanuki
Copy link
Contributor

@CharlesNepote The first time I worked in off-dart - a couple of years ago - the "weight'ed" nutrients had to be converted to grams.
That meant that the user could input something like 5 mg in whatever UI, but for the API it will have to be converted as 0.005 (and no unit).
And if ever on the API we wanted to set a unit (e.g. mg), that had no impact on the value (still needed in grams), but it would just set a "latest default unit".
In our case, I could set 0.005 and mg, which means 0.005 g to be displayed by default in mg (therefore 5 mg in the UI).
I don't know if and how things evolved about those value/unit settings in the API lately.

Points to address:

  1. How come you have tons of default unit in g for the products you attached the screenshots of? Which app created that: web, Smoothie, another app?
  2. Are you able to reproduce this behavior using only off-dart (e.g. I have a normal product with relevant units, but using off-dart it looks like I can force units to g)

Beyond that, not setting the unit for weight'ed nutrients (setting in grams, letting the unit as reasonable default) is only possible for nutrients that have only weight units.
I noticed that for some nutrients in some countries (something like Vitamin B12 in Canada) the default unit is not always a weight (cf. % DV and IU in #905 #906)
In those cases we cannot probably rely on letting the server understand we're talking about grams or IU, and there are changes to work on and to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants