Skip to content

Fix missing constraints error#845

Merged
Bachibouzouk merged 8 commits intodevfrom
fix/adding-constraints
Apr 12, 2021
Merged

Fix missing constraints error#845
Bachibouzouk merged 8 commits intodevfrom
fix/adding-constraints

Conversation

@Bachibouzouk
Copy link
Copy Markdown
Collaborator

@Bachibouzouk Bachibouzouk commented Mar 26, 2021 •
edited
Loading

Fix #827

Changes proposed in this pull request:

  • Loop over the user defined constraints to assign them instead of expecting all possible keys under CONSTRAINTS in dict_values

The following steps were realized, as well (if applies):

  • Use in-line comments to explain your code
  • Write docstrings to your code (example docstring)
  • For new functionalities: Explain in readthedocs
  • Write test(s) for your new patch of code (pytests, assertion debug messages)
  • Update the CHANGELOG.md
  • Apply black (black . --exclude docs/)
  • Check if benchmark tests pass locally (EXECUTE_TESTS_ON=master pytest)

For more information on how to contribute check the CONTRIBUTING.md.

@Bachibouzouk Bachibouzouk requested review from SabineHaas and smartie2076 and removed request for smartie2076 March 26, 2021 16:36
smartie2076
smartie2076 reviewed Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

I see what you did here and it does indeed get rid of the error mesaage and save us some line of code. However, I think that #845 just like #780 is more about the fact that we actually define default values for the constraints, but they are not applied to the inputs:

https://github.com/rl-institut/multi-vector-simulator/blob/fix/adding-constraints/src/multi_vector_simulator/utils/constants.py#L258

There must be something in A1 being off for the default values not being parsed.

I have commented some stuff now. I am not sure this is the preferred way to fix the problem (I do think that we should be able to set default values for the constraints, and I would say it is not intended that you can leave parameters out from the inputs), so I am not sure if you should work more with this hotfix, especially as there are a lot of pytests to be changed/added, right?

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Mar 29, 2021

#834 changed D2 majorly.

@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

Bachibouzouk commented Mar 29, 2021

I think for the constraints it would make sense not to have constraints, rather than having default values to them.

@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

Bachibouzouk commented Mar 29, 2021

#834 changed D2 majorly.

I could reapply my changes on top, the gist of it is to make sure the code runs if the constraints are not defined (also good for back compatibility ;))

smartie2076
smartie2076 reviewed Mar 29, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@smartie2076 smartie2076 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think for the constraints it would make sense not to have constraints, rather than having default values to them.

You mean constraints should be always defined explicitly?

The default values always turn off the constraint, so I think it would not lead to unexpected results.

The gist of it is to make sure the code runs if the constraints are not defined (also good for back compatibility ;))

Yeah, the back compatability is quite an issue. Not sure if trying to ensure that via if-statements within the modules is more efficient though then ensuring that in A1 or B. For the constraints this only seems to require changes in D2 and E0, but for others this may be all over the place.

If we want the hotfix, surely avoid excessive constraint preparation time and not change the pytests for the constraints, we could also simply change to the comment below.

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Apr 6, 2021

As you say that this will make sure that MVS stays downward compatible, lets use this PR to evade some issues. I would say that it does not really fix the issue with the parameters, but we have two similar issues anyway (#780 and #827), so one of them can be closed with this.

@Bachibouzouk Bachibouzouk force-pushed the fix/adding-constraints branch from c52ed56 to 6dddc31 Compare April 9, 2021 13:11
@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

Bachibouzouk commented Apr 12, 2021

The error (missing Key in the last test of test_D0) originate from a part of code which was in dev, @ciaradunks do you know how it should be fixed?

@smartie2076
Copy link
Copy Markdown
Collaborator

smartie2076 commented Apr 12, 2021

Hi @Bachibouzouk! I think the issue is that now, with your changes, you do your pre-processing with D0.prepare_constraint_minimal_renewable_share() first and then evaluate the constraint. Was this intentional?

Basically, before that the test was able to skip the constraints and run the test on tests/test_data/inputs_for_D0/mvs_config.json. There, energyVector is missing from the evaluated energyProduction consumption source:

grafik

So, you can either add an energyVector to this, or you move the check for the value of the constraint before D0.prepare_constraint_minimal_renewable_share(), if that is possible.

This is a reason why I think that D0, D1 would do better if we would not test them with a json file but with input csv files...

smartie2076
smartie2076 approved these changes Apr 12, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

I think you should rather move the pre-processing into if-loop. It seems that would not cause any problems.

Are you intentionally not writing with functions you changed in the CHANGELOG? Especially that the validity checks are only performed if a constraint is actually added is also useful information.

@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

Bachibouzouk commented Apr 12, 2021

I think you should rather move the pre-processing into if-loop. It seems that would not cause any problems.

Are you intentionally not writing with functions you changed in the CHANGELOG? Especially that the validity checks are only performed if a constraint is actually added is also useful information.

Not writing the function names in the CHANGELOG was not intentional, merly laziness I would guess, I will update when I fix

@Bachibouzouk Bachibouzouk force-pushed the fix/adding-constraints branch from c0c7629 to 01d77f4 Compare April 12, 2021 19:45
@Bachibouzouk Bachibouzouk merged commit af6e5ea into dev Apr 12, 2021
@Bachibouzouk Bachibouzouk deleted the fix/adding-constraints branch April 12, 2021 19:48
@smartie2076 smartie2076 mentioned this pull request May 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartie2076 smartie2076 smartie2076 approved these changes

@SabineHaas SabineHaas Awaiting requested review from SabineHaas

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[bug] KeyError: 'net_zero_energy'

2 participants

@Bachibouzouk @smartie2076