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

Revise/nx graph #439

Merged
merged 7 commits into from Jan 22, 2018

Conversation

Projects
None yet
2 participants
@uvchik
Copy link
Member

uvchik commented Jan 17, 2018

Revision of the graph module

  • I could not see the advantage of using a solph model instead of the energy system as the solph model gets its information from the energy system. That's why I removed the option to pass a model instead of an energy system.
  • I added a label to the nodes which helps to show the description in external tools
  • I added the weight to edges if an energy system is passed
  • I added the option to write the graph to disc as an .graphml file.

Things we may want to discuss

  • Do you like the integrated "write" option or do want to leave it to the user?
  • At them moment no "weight" attribute is written if the weight is None. We also could add an empty attribute. I think both ways have advantages.

@uvchik uvchik added this to the v0.2.1 milestone Jan 17, 2018

@uvchik uvchik self-assigned this Jan 17, 2018

@uvchik uvchik requested review from simnh and jnnr Jan 17, 2018

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Jan 18, 2018

I had to fix my PR, because I forgot that we are not allowed to change the API for v0.2.1.
The optimisation_model attribute will now raise a FutureWarning, but will still work.
Here are the Travis results: https://travis-ci.org/oemof/oemof/builds/330299021

@jnnr

This comment has been minimized.

Copy link
Contributor

jnnr commented Jan 18, 2018

Looks good and gives nice results. Thanks! I like the labeling and weights. The integrated write function can be useful sometimes. If not needed, one just does not pass a filename, so that's perfectly ok.

The docstring could be revised. Under "Parameters", "filename" appears twice. Should I fix it?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Jan 18, 2018

The docstring could be revised. Under "Parameters", "filename" appears twice. Should I fix it?

Are you sure you have the latest commit?

@jnnr

This comment has been minimized.

Copy link
Contributor

jnnr commented Jan 18, 2018

Somehow I didn't. Strange. But now its ok.

@jnnr

jnnr approved these changes Jan 18, 2018

@uvchik uvchik requested a review from jnnr Jan 18, 2018

@jnnr

jnnr approved these changes Jan 22, 2018

@uvchik uvchik merged commit 701fff9 into dev Jan 22, 2018

@uvchik uvchik deleted the revise/nx_graph branch Jan 22, 2018

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