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

Added energysystem compatibility for param_results #445

Merged
merged 3 commits into from Feb 7, 2018

Conversation

Projects
None yet
3 participants
@henhuy
Copy link
Contributor

henhuy commented Feb 2, 2018

Just a little addition to former add-node-param-obj-results PR:

Now, not only (Operational-)Model, but also the energysystem can be
given as input parameter for processing.param_results function

Added energysystem compatibility for param_results
Now, not only (Operational-)Model, but also the energysystem can be
given as input parameter for processing.param_results function

@henhuy henhuy added the enhancement label Feb 2, 2018

@henhuy henhuy self-assigned this Feb 2, 2018

@henhuy henhuy requested review from uvchik and ckaldemeyer Feb 2, 2018

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Feb 2, 2018

It's only a simple feature - I did not know if I can push directly to dev...

Parameters
----------
system: Model or energysystem

This comment has been minimized.

@ckaldemeyer

ckaldemeyer Feb 5, 2018

Member

I would suggest "Model or EnergySystem" or "optimization model or energy system"

@ckaldemeyer
Copy link
Member

ckaldemeyer left a comment

If the docstring is changed, I am fine. Thanks!

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Feb 5, 2018

I would say solph.model or solph.EnergySystem, because it is meant an instance of the solph.EnergySystem class and not any energy system.

By the way I would not allow passing a model but only(!) EnergySystem. Models gets their informations from the EnergySystem so this is the source. Models might vary in the future so it will be additional work to keep both APIs in line. So I would not add the option to pass an ES but replace the option to pass a Model.

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Feb 5, 2018

By the way I would not allow passing a model but only(!) EnergySystem.

I thought I shouldn't change the API after a release?
That's the only reason I allowed both options...

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Feb 5, 2018

Ah okay, I did not know that this is already part of v0.2.0, I thought this is something new.

Anyhow, we should talk about the usage of Models and Systems on the next developer meeting.

@henhuy henhuy merged commit f2fa4cf into dev Feb 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment