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

Naming consistency of solph examples and change all solvers to 'cbc' #238

Merged
merged 23 commits into from Nov 10, 2016

Conversation

Projects
None yet
4 participants
@simnh
Copy link
Member

simnh commented Nov 7, 2016

No description provided.

@simnh simnh added the oemof-solph label Nov 7, 2016

@simnh simnh added this to the v0.1.2 milestone Nov 7, 2016

@simnh simnh self-assigned this Nov 7, 2016

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 7, 2016

Could we rename this PR to "Consistency of solph examples" and change all solvers to CBC.

We already decided the we recommend the CBC solver, so I think it makes things easier, if we all examples work with one solver. To test the GLPK we could add an argument to run the examples using GLPK instead of CBC. But CBC should be the default.

Do you agree?

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 7, 2016

@gplssm is it possible to pass an argument to the examples script to allow changing the solver. The default would then be the cbc:

oemof_examples simple_least_costs -s glpk

@simnh simnh changed the title Naming consistency of solph examples Naming consistency of solph examples and change all solvers to 'cbc' Nov 7, 2016

@simnh

This comment has been minimized.

Copy link
Member Author

simnh commented Nov 7, 2016

I renamed it but i wont rename the branch if thats ok

@gplssm

This comment has been minimized.

Copy link
Contributor

gplssm commented Nov 7, 2016

Yes, it is possible to include an optional argument to specify the solver. I'll do it.

Other wished regarding names of examples and so on?

@gplssm gplssm self-assigned this Nov 7, 2016

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 7, 2016

I renamed it but i wont rename the branch if thats ok

Certainly.

simnh added some commits Nov 7, 2016

@simnh

This comment has been minimized.

Copy link
Member Author

simnh commented Nov 8, 2016

I did it, please check it out and finalize if there is anything still missing.

@simnh simnh assigned uvchik and unassigned simnh Nov 8, 2016

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 8, 2016

I will push my additions soon.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 8, 2016

What I did:

  • added a solver parameter or **kwargs to make it possible to change the solver in the examples script (08d25dd)
  • cleaned up the examples according to our developer rules (we should be a good example 😏) (184d17a)
  • change the names in the examples script to the new names ( 38fbce6)
  • added a user feedback to the add_constraints example because it ended without any output (2ce6889)

I also added the possibility to run the storage_investment example without matplotlib (8725bd7). We do not have to do it, but I think in the long run we should remove the matplotlib from oemof's requirements. It is just an example how we can do it, without changing the code a lot.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 8, 2016

What about the csv-examples?

  • csv_reader_dispatch
  • csv_reader_investment
@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 8, 2016

Yes. But I'll probably come to that until Thursday

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

Shall we leave the 'optimization' e.g. in 'storage_capacity_optimization'?

I would vote for yes!

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

Ah, already done... Forget my comment...

ckaldemeyer added some commits Nov 9, 2016

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 9, 2016

Okay I fixed it, but the result check for the dispatch example fails since commit 515e251.

14:12:36-ERROR-R2_storage_phs: 93019.19636 not between 88822.57254397201 and 89000.395512028
14:12:36-ERROR-objective: 2204738892.3982296 not between 2323929476.7974014 and 2328581988.2624617
14:12:36-ERROR-R2_R1_powerline: 2418937.89885 not between 2275711.011 and 2280266.989

I just wonder why the efficiency of the some power plants was changed (eg. nuclear power plant 0.325 -> 325) with commit 515e251.

@ckaldemeyer However, please check which version is right and adapt the results.

I added the investment example to the example tests. As there are no results it is only a run check but no result check. As we see above it is worth adding a result check.

@simnh

This comment has been minimized.

Copy link
Member Author

simnh commented Nov 9, 2016

this remembers me of maybe introducing checks for flow attributes inside the csv-reade (min=0, max=1)...If you agree @ckaldemeyer please create an issue and we can add this. the best way of course would be to introduce it inside the flow class (but for after instantiating adding of attributes as well)

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

I just wonder why the efficiency of the some power plants was changed (eg. nuclear power plant 0.325 -> 325) with commit 515e251.

My mistake! I re-installed my system and my libreoffice calc locale was set to german. So just saving the csv file misinterpreted the english decimal sign (.) as a thousand sign..

Thanks for your effort!

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

The dispatch example should work now.

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 9, 2016

@ckaldemeyer It is good to have result tests 😏 - works.

Could we rename the examples in the examples.py from "investment" to csv-reader-investment? The same for dispatch?

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

@ckaldemeyer It is good to have result tests 😏 - works.

Definitively!

Could we rename the examples in the examples.py from "investment" to csv-reader-investment? The same for dispatch?

I am fine with it!

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 9, 2016

The output of your csv-examples is "Check the results".

I would prefer something like "The results can be found in {0}".format(result_path)

BTW: It is strange to add line breaks within logging messages.

15:42:03-INFO-Something to say....
15:42:04-INFO-I wanted to add a line break
So I did
15:42:07-INFO-Result are stored....
@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

I don't mind at all. I'll just fix it..

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 9, 2016

I don't mind at all. I'll just fix it..

It is just for users executing the script. They do not see the code and "Check the results" will say nothing to them.

I am fine now. I would just wait for @gplssm to adapt the example script and then merge it.

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

It is just for users executing the script. They do not see the code and "Check the results" will say nothing to them.

No worries, you are right ;-) I just didn't notice..

I fixed it now in the last commit. Together with an error that was induced by a redundant folder 'scenarios' in the file path.

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Nov 9, 2016

Oh, it didn't fix it but just fixed the error message. I'll look at it again..

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Nov 10, 2016

Okay, I think everything works fine now.

@ckaldemeyer If you change the shape of the results you could remove the following logging message;

11:32:20-INFO-Read the documentation (outputlib) to learn how to process the results.
@simnh

This comment has been minimized.

Copy link
Member Author

simnh commented Nov 10, 2016

Ok, I will merge for now but we might add some text to the whats new? (maybe)

@simnh simnh merged commit 93686b9 into dev Nov 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@uvchik uvchik deleted the features/naming-consistency-of-examples branch Nov 10, 2016

@uvchik uvchik referenced this pull request Nov 10, 2016

Merged

Revise Example #247

10 of 13 tasks complete

gplssm added a commit that referenced this pull request Nov 13, 2016

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