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

Features/add min max runtimes #446

Merged
merged 29 commits into from
Mar 7, 2018
Merged

Conversation

ckaldemeyer
Copy link
Member

This adds the possibility to add min and max runtimes for solph.NonConvexFlows.

See oemof/oemof-examples#16 for two example usages!

@ckaldemeyer
Copy link
Member Author

ckaldemeyer commented Feb 8, 2018

Memo:

  • Write constraint test
  • Add equations
  • Remove if statements if m.flows[i, o].nonconvex.minimum_downtime is not None from rules
  • Adapt release notes
  • Allow for time-varying start-/shutdown costs e.g. due to varying resource prices

@ckaldemeyer
Copy link
Member Author

Memo:

Write constraint test
Add equations
Remove if statements if m.flows[i, o].nonconvex.minimum_downtime is not None from rules
Adapt release notes
Allow for time-varying start-/shutdown costs e.g. due to varying resource prices

I have now realized except for the constraint test. If it has been added, it should be fine!

@ckaldemeyer ckaldemeyer requested review from simnh, uvchik and jnnr February 9, 2018 16:51
@ckaldemeyer
Copy link
Member Author

@uvchik
add constraint test

Looks good!

@ckaldemeyer
Copy link
Member Author

I would like to have this in the next release so could you also review @simnh and @jnnr ?

@ckaldemeyer
Copy link
Member Author

I would like to have this in the next release so could you also review @simnh and @jnnr ?

You could also review the examples as they illustrate the functionality: oemof/oemof-examples#16

@jnnr
Copy link
Member

jnnr commented Mar 6, 2018

Good feature! I reviewed the examples, see PR #16. The actual code seems to have a problem passing the tests. There is an error in constraint_tests.py in test_emission_constraints().

@ckaldemeyer
Copy link
Member Author

The actual code seems to have a problem passing the tests. There is an error in constraint_tests.py in test_emission_constraints().

This is weird. The tests have passed with oemof/oemof@1795b1d and started to fail with oemof/oemof@f2dfb12 although the changes only affected the documentation. And the emission constraints are not related to the runtimes..

@simnh
Copy link
Member

simnh commented Mar 6, 2018

I can not review the correctness of this new functionality atm, Source code looks fine. Only thing was the update of docstrings (I pushed it).

@ckaldemeyer: Thanks for adding!!

@uvchik
Copy link
Member

uvchik commented Mar 6, 2018

This is a pyomo issue. It comes with 5.4.x. Just add pyomo<5.4 to the setup.py.

Then we should open a new issue to find out if it is a bug or an API change.

@ckaldemeyer
Copy link
Member Author

I can not review the correctness of this new functionality atm, Source code looks fine. Only thing was the update of docstrings (I pushed it).

@ckaldemeyer: Thanks for adding!!

Thanks!

@uvchik
Copy link
Member

uvchik commented Mar 6, 2018

See #448.

@ckaldemeyer
Copy link
Member Author

This is a pyomo issue. It comes with 5.4.x. Just add pyomo<5.4 to the setup.py.

Then we should open a new issue to find out if it is a bug or an API change.

Thanks for pointing this out. Nevertheless, this PR should be mergeable now as @simnh @jnnr and you are fine with it and the Pyomo error is of general nature. @jnnr : Feel free to merge if you are o.k. with my changes!

@uvchik
Copy link
Member

uvchik commented Mar 6, 2018

Okay, I limited the pyomo version to pyomo < 5.4 until we know what happened there.

Now the tests are fine 😄

@ckaldemeyer ckaldemeyer merged commit 15d9335 into dev Mar 7, 2018
@uvchik uvchik deleted the features/add-min-max-runtimes branch March 7, 2018 09:47
@jnnr
Copy link
Member

jnnr commented Mar 7, 2018

Very good. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants