-
Notifications
You must be signed in to change notification settings - Fork 123
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
figure.py: make plotly an optional dependency #549
Conversation
plotly is used in only one function, make it an optional dependency
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 Line 50 in 6435898
Also, you'll have to add the new dependency type to all GitHub actions workflow install steps, see pyam/.github/workflows/pytest.yml Line 39 in 6435898
(and the same in the other workflow recipes). |
Hi @danielhuppmann , thanks for your comments, I have pushed commits addressing them, please let me know if I missed something. |
Thanks! Letting the tests do their thing... |
The workflow recipe for building the docs also needs the new optional plotting dependency so that it can build the gallery. pyam/.github/workflows/build-docs.yml Line 39 in 6435898
And the ReadTheDocs config will then also need that... https://github.com/IAMconsortium/pyam/blob/main/readthedocs.yml |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Update: - build-docs workflow - RTD config
Done :) |
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? |
Thanks, I feel this is too trivial a change for a mention in the Authors list :-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @suvayu for resolving dependency conflicts between pyam and some of the modelling frameworks out there!
Please confirm that this PR has done the following:
Tests AddedDocumentation AddedName of contributors Added to AUTHORS.rstDescription 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 withplotly
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 infigures.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?