multi-objective weights #25
Conversation
|
@stefansc1 - could you solve the merge conflicts with the latest dev version? @JulianBarinton - here I think you are better tooled than me to judge this PR |
paulapreuss
left a comment
paulapreuss
left a comment
There was a problem hiding this comment.
Thank you! 🚀
Looks good, I tested and it all seems to work well. I added some comments about suggestions that I think would make for a slightly cleaner implementation, but it is to be understood more friendly commentary and not necessary to change the implementation now, since the functionality is fine.
| <div> | ||
| {{ form.total_cost.label }}{{ form.total_cost }} | ||
| {{ form.co2_emissions.label }}{{ form.co2_emissions }} | ||
| {{ form.land_requirements.label }}{{ form.land_requirements }} | ||
| {{ form.water_footprint.label }}{{ form.water_footprint }} | ||
| </div> | ||
| <div class="no-bullet error"> | ||
| {{ form.non_field_errors }} | ||
| </div> |
There was a problem hiding this comment.
I would suggest just wrapping the entire form in a crispy tag here. It looks nicer, already displays the validation error and saves a couple lines. I will push a commit with it.
app/wefe/forms.py
Outdated
| class MOOForm(forms.Form): | ||
| # multi-objective optimization setup | ||
| total_cost = forms.FloatField( | ||
| min_value=0, max_value=1, initial=1, | ||
| widget=forms.NumberInput(attrs={'step': 0.1, 'default': 1}) | ||
| ) | ||
| co2_emissions = forms.FloatField( | ||
| min_value=0, max_value=1, initial=0, | ||
| widget=forms.NumberInput(attrs={'step': 0.1, 'default': 0}) | ||
| ) | ||
| land_requirements = forms.FloatField( | ||
| min_value=0, max_value=1, initial=0, | ||
| widget=forms.NumberInput(attrs={'step': 0.1, 'default': 0}) | ||
| ) | ||
| water_footprint = forms.FloatField( | ||
| min_value=0, max_value=1, initial=0, | ||
| widget=forms.NumberInput(attrs={'step': 0.1, 'default': 0}) | ||
| ) | ||
|
|
There was a problem hiding this comment.
This works but it is a bit redundant to define the form fields after having defined the model - you could have also automatically created the form through
class MOOForm(ModelForm):
class Meta:
model = MOOWeights
exclude = ["scenario"]Then your form would clealy be connected to the MOOWeights model and you would only need to tweak the step size for the widgets. Just a suggestion for a (in my opinion) cleaner implementation though, this works and we can leave it as is.
app/wefe/forms.py
Outdated
| land = cleaned_data.get("land_requirements") | ||
| water = cleaned_data.get("water_footprint") | ||
| if cost is not None and co2 is not None and land is not None and water is not None: | ||
| if cost + co2 + land + water != 1: |
There was a problem hiding this comment.
I got some validation errors because of rounding errors (e.g. if I input 0,5 + 0,3 + 0,2 it calculated 0.99999...), so I suggest implementing a small rounding safeguard after a few decimals.
| if cost + co2 + land + water != 1: | |
| if round(cost + co2 + land + water, 4) != 1: |
| try: | ||
| # weights already defined: get values | ||
| weights = scenario.mooweights | ||
| context["form"] = MOOForm(initial=vars(weights)) | ||
| context["custom_weights"] = weights.total_cost != 1 | ||
| except ObjectDoesNotExist: | ||
| # no weights yet: use defaults | ||
| context["form"] = MOOForm() |
There was a problem hiding this comment.
Again, this works as it is, but I don't know about using a except: for a condition that is very normal within the code like no MOO set yet (not an unexpected exception). Instead of waiting for an Exception to be raised I think it would be cleaner/less computationally expensive to formulate this as:
weights_qs = MOOWeights.Objects.filter(scenario=scenario)
if weights_qs.exists():
weights = scenario.mooweights
# ...
else:
# no weights yet...| MOOWeights.objects.update_or_create( | ||
| scenario=scenario, | ||
| defaults={ | ||
| "total_cost": form.cleaned_data["total_cost"], | ||
| "co2_emissions": form.cleaned_data["co2_emissions"], | ||
| "land_requirements": form.cleaned_data["land_requirements"], | ||
| "water_footprint": form.cleaned_data["water_footprint"], | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Continuing on the point about using a ModelForm instead of an unconnected Model and Form, if you had a ModelForm you could simply save the cleaned form data to the model like this
if form.is_valid():
moo = form.save(commit=False) # creates a model instance with the data from the modelform fields
moo.scenario = scenario # add the information that was not caught in the form
moo.save()
|
I made the form crispy and adjusted a layout a bit for aesthetic reasons in #c2e9324, hope that's fine by you. You can have a look at it. |
|
@stefansc1 There are some conflicts now (sorry, that's my bad for letting this PR sit for so long). you probably need to rebase onto the newest |
Scenario step 6: add multi-objective optimization weights. May be skipped, with single optimization for costs. Weights are linked 1-to-1 with scenario.