-
Notifications
You must be signed in to change notification settings - Fork 33
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
Flexible setup of constraints for cooling technologies #242
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
=======================================
- Coverage 76.4% 75.5% -1.0%
=======================================
Files 203 203
Lines 15503 15545 +42
=======================================
- Hits 11855 11747 -108
- Misses 3648 3798 +150
|
This comment was marked as resolved.
This comment was marked as resolved.
Thanks Frido and Meas. I am not sure why this error appears, but I also had it on my machine, probably to do with some pandas update?
|
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.
All in all, good work as far as the coding goes :)
However, I'd like to see several small improvements and please check why the tests are failing on this PR and ensure they don't do that.
To figure this out, you can recreate a virtual environment locally that has the same pandas and numpy and Python versions installed as one of the failing workflows. Then, try to run the tests in there and use a debugger or add auxiliary output What the error message says is that |
move from hard-coded contraints to automatically set, to avoid infeasibilities
this was is form another older branch
de1d013
to
2f9625f
Compare
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 sorry, I tried to rebase this branch on main
to keep it up to date (since some of @measrainsey's branches were merged earlier today), but this seems to have broken the tests again.
Thanks for fixing them before! If we fix them now and add this PR to doc/whatsnew
(and @measrainsey's review comes in), I think we're close to getting this merged :)
The errors were caused by updating |
@adrivinca given the name of this branch ( |
2f9625f
to
22e23ce
Compare
22e23ce
to
92a61bb
Compare
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 to rebase this branch on itself to reword the commit messages, but that accidentally picked up several commits not belonging here, not entirely sure why. So I'll not use that route; instead, we should squash and merge
this PR, providing a fitting commit message.
For the future: @adrivinca, we try to have all commit messages adhere to the same form: using present tense to describe why changes are made more than what changed (which should be apparent from the code) in a short sentence. So e.g. "Fix breaking test" rather than "Fixing breaking test".
I also added this PR to doc/whatsnew
, so I think we can merge it now, if you like. And I would appreciate not adding many things/anything of significance to a PR after it has been reviewed and approved unless the review asked for specific things.
The PR changes some hard code to a flexible adjustment of constraints on cooling technologies, to improve flexibility
The cooling technologies are added as add-on to power plants (parent technologies). in some scenarios, however, the power plant are subject to constraints e.g. for phase-out, and the cooling tecnhologies
growth_activity_lo
need to be adjusted accordingly to avoid infeasibilities. Before this was done manually, now I added a functionrelax_growth_constraint
that should do it automatically for each specific scenario.I also added a test function to test the behavior of the function.
How to review
Check the code and test are in line with the standards.
Eventually run the code on a scenario at choice and see it the model solves
mix-models --url=ixmp://ixmp_dev/%model%/%scenario% water-ix --regions=R12 cooling
, maybe @measrainseyPR checklist