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

chore(data): New ingredients #814

Open
wants to merge 96 commits into
base: master
Choose a base branch
from
Open

chore(data): New ingredients #814

wants to merge 96 commits into from

Conversation

ccomb
Copy link
Collaborator

@ccomb ccomb commented Oct 24, 2024

🔧 Problem

We're missing many ingredients.

🍰 Solution

Added ingredients and exported the impacts.

Along with the new ingredients, some other modifications that needed to be done to get a more reliable result:

  • The brightway search now makes sure there is only one result. This forces to use more precise search terms to find a process. If it's impossible to get only one results, setting the search field to the exact name of the process forces to select this exact process.
  • The full list of activities.json has also been adapted to this behaviour

Since the ingredients editor is used when adding ingredients, it also has been adapted and fixed:

  • It has been adapter to this search behaviour, and the local save button is disabled until the search term leads to a single result.
  • It also now allows to set empty ecosystemic services, which was needed while adding new ingredients
  • Some select fields of the ecosystemic services have been translated
  • A bug has been fixed regarding the new plural categories fields (process_category)

🏝️ How to test

Check that the new ingredients on the review app are what is expected (naming, impacts, identifier, etc.)

ecobalyse-private: ingredients

Ingredient editor and others added 30 commits July 25, 2024 09:20
c Veuillez saisir le message de validation pour vos modifications. Les lignes
display the ecoscore and columns with percentage
Copy link
Member

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just reviewing updated Elm code here. I'll leave others validating the rest of the changes.

Comment on lines -171 to +174
|> Expect.within (Expect.Absolute 0.01) 478.0375489673356
|> Expect.within (Expect.Absolute 0.01) 478.03754896733557
|> asTest "should properly score total impact"
, Unit.impactToFloat scoring.allWithoutComplements
|> Expect.within (Expect.Absolute 0.01) 476.8371815514294
|> Expect.within (Expect.Absolute 0.01) 476.83718155142935
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expect.Absolute 0.01 means we have a 0.01 tolerance for deviations. Why did you bother updating these two values which are both well within this tolerance? You were getting test failures here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot why. The probable reason is I once got a very different result which made me update the test, then got back a similar one which would not have make the test fail

Copy link
Collaborator

@vjousse vjousse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some nits. I can't really tell if it works or not, I don't understand what I should test and how.
Having a detailed "How to test" section could help here.
On a side note: this PR should have been split in three or more PRs. Searching for unique results, dealing with ecosystemic services, fixing the plural bug and fixing the export are separate things to me and it would have ease the review process.

data/notebooks/ingredients.py Outdated Show resolved Hide resolved
):
if x in a:
del activities[i][x]
# _ = activities[i].pop(x, None)
else:
# remove empty SE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an SE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Service Ecosystemique

data/notebooks/ingredients.py Show resolved Hide resolved
# ingredients attributes
"categories": "Catégories d'ingrédient",
"ingredient_categories": "Catégories d'ingrédient",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"ingredient_categories": "Catégories d'ingrédient",
"ingredient_categories": "Catégories d'ingrédients",

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, you're editing a single ingredient

data/notebooks/ingredients.py Outdated Show resolved Hide resolved
w_scenario = ipywidgets.Dropdown(options=["reference", "organic", "import"], value=None)

# buttons
savebuttontooltip = "Enregistre l'ingrédient créé ou modifié"
savebuttontooltipnonunique = "Vos termes de recherche doivent donner un seul résultat"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
savebuttontooltipnonunique = "Vos termes de recherche doivent donner un seul résultat"
save_button_tooltip_non_unique = "Vos termes de recherche ne doivent donner qu’un seul résultat"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

data/notebooks/ingredients.py Outdated Show resolved Hide resolved
data/notebooks/ingredients.py Outdated Show resolved Hide resolved
data/notebooks/ingredients.py Show resolved Hide resolved
@ccomb
Copy link
Collaborator Author

ccomb commented Nov 12, 2024

Added some nits. I can't really tell if it works or not, I don't understand what I should test and how. Having a detailed "How to test" section could help here. On a side note: this PR should have been split in three or more PRs. Searching for unique results, dealing with ecosystemic services, fixing the plural bug and fixing the export are separate things to me and it would have ease the review process.

Thanks for your suggestions!
I've really tried to keep the PR as minimal as possible, but it was done during real usage by Julia (so actually in "production" on the ingredients branch which is a container for new ingredients), with an export leading to process selection issues (unique results) that needed to be fixed in the same PR. Same for exosystemic services which were edited during the same process in the same branch. @paulboosz told there is another needed change : replacing manual ids with uuids. But this one can really be done in another PR.

"name": "Winter pea, conventional, 15% moisture, at farm gate {FR} U",
"source": "Agribalyse 3.1.1",
"system_description": "AGRIBALYSE",
"name": "Lentils, dry, at farm (WFLDB)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Winter pea is replaced by Lentils. I guess it's more adapted to the id "lentils-uncooked-fr". Is there any explanation why we had winter pea before ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no information found about this choice

@@ -7550,7 +7516,7 @@
{
"categories": ["ingredient"],
"comment": "",
"displayName": "Blé tendre Hors UE Conv.",
"displayName": "Blé tendre UE Conv.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the id is soft-wheat-non-eu and we're changing the displayName from Hors UE to UE but the LCI stays Soft wheat GLO. Is that normal ?

(Now the displayName is incoherent with the id, we really need to go to uuid)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Julia has found the error and will fix it

@ccomb ccomb requested a review from JFraichou November 15, 2024 09:49
Copy link
Collaborator

@JFraichou JFraichou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed data/food/activities.json and fixed the soft-wheat-eu and the onion-dried.
Every changes has been listed and will be written down on a dedicated Notion card

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

Successfully merging this pull request may close these issues.

5 participants