-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: some fixes and experiments #15
Conversation
@@ -41,6 +41,31 @@ def add_ingredients_to_solver(ingredients, solver, total_ingredients): | |||
|
|||
return ingredient_numvars | |||
|
|||
# Add constraints to ensure that the quantity of each ingredient is greater than or equal to the quantity of the next ingredient | |||
# and that the sum of children ingredients is equal to the parent ingredient | |||
def add_relative_constraints_on_ingredients(solver, parent_ingredient_numvar, ingredient_numvars): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this is a good way to do it. It might be better just to keep ingredients without a CIQAL code completely out of the optimisation and then just add them back in at the end, half way between their greater and lesser ingredient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we do need the solver to know about ingredients without CIQUAL code even if we consider that they don't have any nutrient contribution, as they add constraints on the other ingredients.
e.g. if we have "unknown, sugar, oil", then sugar must be 50% or less.
def get_quantity_estimate(ingredient_numvars): | ||
total_quantity = 0 | ||
quantity_estimate = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what these changes are trying to do but they have broken the unit tests around quantity_estimates and evaporation. The idea of the quantity_estimate is that it is the amount of original ingredient that is needed to make 100g of the product, i.e. the quantity before evaporation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still the same definition of quantity_estimate, except that it adds quantity estimates for parent ingredients as well.
continue | ||
|
||
|
||
computed_nutrient['weighting'] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a permanent change or just an experiment? Breaks a unit test (I have commented the test for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried many values and looked at the resulting accuracy: https://docs.google.com/document/d/1NFg2lAaDzSkRldKU6uDGcjPVxbfTsc9p59r7L6OEqiQ/edit#heading=h.fl6o8g6m5uv5
So far it's the best I found.
Metrics are much better with this branch, so I will merge it. reran model on recipe estimator (main branch)
|
Main changes:
e.g. for Nutella: