IAMconsortium / pyam

Analysis & visualization of energy & climate scenarios
https://pyam-iamc.readthedocs.io/
Apache License 2.0
234 stars 120 forks source link

figure.py: make plotly an optional dependency #549

Closed suvayu closed 3 years ago

suvayu commented 3 years ago

Please confirm that this PR has done the following:

Description of PR

With this PR, I am proposing to make plotly an optional dependency.

One of the feedbacks I got for friendly data (which uses pyam) was that there's a version mismatch with plotly with their environment, preventing them from using the Python API directly from their analysis code.

I noticed that plotly is used only in one place in figures.py, so I thought maybe this is an acceptable proposal. Since it's easier to talk in code, I wrote this PR. If I have misunderstood the importance of that module, please feel free to close this.

Thoughts?

danielhuppmann commented 3 years ago

Thanks @suvayu, don't see an issue with making plotly an optional dependency. But I'm not sure if the current implementation works, I suggest to rather follow the approach used in the datareader module, see https://github.com/IAMconsortium/pyam/blob/64358981fcea736ee5f5110ce6a89cd559411090/pyam/datareader.py#L50

Also, you'll have to add the new dependency type to all GitHub actions workflow install steps, see https://github.com/IAMconsortium/pyam/blob/64358981fcea736ee5f5110ce6a89cd559411090/.github/workflows/pytest.yml#L39 (and the same in the other workflow recipes).

suvayu commented 3 years ago

Hi @danielhuppmann , thanks for your comments, I have pushed commits addressing them, please let me know if I missed something.

danielhuppmann commented 3 years ago

Thanks! Letting the tests do their thing...

danielhuppmann commented 3 years ago

The workflow recipe for building the docs also needs the new optional plotting dependency so that it can build the gallery.

https://github.com/IAMconsortium/pyam/blob/64358981fcea736ee5f5110ce6a89cd559411090/.github/workflows/build-docs.yml#L39

And the ReadTheDocs config will then also need that...

https://github.com/IAMconsortium/pyam/blob/main/readthedocs.yml

codecov[bot] commented 3 years ago

Codecov Report

Merging #549 (18acff0) into main (6435898) will decrease coverage by 0.1%. The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #549     +/-   ##
=======================================
- Coverage   93.6%   93.4%   -0.2%     
=======================================
  Files         48      48             
  Lines       5299    5301      +2     
=======================================
- Hits        4961    4955      -6     
- Misses       338     346      +8     
Impacted Files Coverage Δ
setup.py 0.0% <ø> (ø)
pyam/figures.py 38.0% <100.0%> (+6.5%) :arrow_up:
pyam/datareader.py 50.0% <0.0%> (-50.0%) :arrow_down:
tests/test_datareader.py 69.5% <0.0%> (-13.1%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6435898...18acff0. Read the comment docs.

suvayu commented 3 years ago

Done :)

danielhuppmann commented 3 years ago

Thanks, looks good! I marked the first two to-do items in the PR description as not applicable - do you want to add your name to the list of contributors?

suvayu commented 3 years ago

Thanks, I feel this is too trivial a change for a mention in the Authors list :-p