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

Sankey plot #466

Merged
merged 12 commits into from
Dec 17, 2020
Merged

Sankey plot #466

merged 12 commits into from
Dec 17, 2020

Conversation

mabudz
Copy link
Collaborator

@mabudz mabudz commented Dec 15, 2020

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

This PR introduces a function sankey() to create a basic Sankey diagram using plotly [https://plotly.com/python/sankey-diagram/]. This figure can be further modified (e.g. fig.update_traces()) or used (e.g. fig.write_html()) by the user [https://plotly.com/python/graph-objects/]

The function uses a mapping dictionary {'variable1': ('source1', 'target1'), ...} to specify the source and the target node for the considered variables.

Currently only one region and one year can be plotted.

@danielhuppmann
Copy link
Member

there was an update of the sphinxcontrib-bibtex package and a change of the WorldBank data used in the pandas_datarader test, which currently causes unit tests to fail - don't worry about that for now

@mabudz
Copy link
Collaborator Author

mabudz commented Dec 15, 2020

Alright. I just address the recommendations of picky Mr.Sticklers

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mabudz for this nice addition! a few suggestions to clean up the implementation... Also, I resolved all issues by stickler that you have implemented, but there are still a few stickler requests...

@danielhuppmann
Copy link
Member

I also just realized that you need to add plotly to the dependencies explicitly, i.e. here

@mabudz
Copy link
Collaborator Author

mabudz commented Dec 16, 2020

Ah, I forgot to add the plotly dependencies. Should not do this while sitting on the airport :)))

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor suggestion for a formatting clean-up inline, and please add to the release notes - then good to go! As discussed, we'll tackle an example for the plotting gallery and tests during/after the holidays...

@danielhuppmann
Copy link
Member

Merging and tackling the tests in a next iteration - thanks @mabudz!

@danielhuppmann danielhuppmann merged commit 7470ddd into IAMconsortium:master Dec 17, 2020
@mabudz mabudz deleted the sankey_plot branch December 20, 2020 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants