-
Notifications
You must be signed in to change notification settings - Fork 471
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
[minor] Add static plotly backend #1286
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #1286 +/- ##
==========================================
+ Coverage 89.82% 89.84% +0.01%
==========================================
Files 38 38
Lines 5142 5149 +7
==========================================
+ Hits 4619 4626 +7
Misses 523 523
|
…t default static display type to svg
@@ -339,6 +347,8 @@ def plot_components( | |||
for ax in multiplicative_axes: | |||
ax = set_y_as_percent(ax) | |||
unregister_plotly_resampler() | |||
if plotly_static: |
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.
@hxyue1 I am wondering, if in the 'plotly_static' case, we will display the figure twice if calling another .show() when applying the plot function?
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.
Hey @hxyue1! Thanks a lot for your work in this PR. Looks generally good to me. :)
3 points to touch upon:
- if we call a
fig.show()
in the 'plotly_static' case and return thefig
, do we run in a problem of plotting it twice with this function call. - can you edit the
test_plot()
in the test_plotting.py
with testing the 'plotly_static' ? I think it would be good the have it checked via at least one test. Editing all tests would be a little much. Would be great to also add theplotly_resampler
on that go - Can as well adjust the plotly tutorial with the update of the
plotly_static
backend?
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.
Great work! Thanks for keeping up with it
@hxyue1 please have a final look at resolving the merge conflict. Afterwards, feel free to merge. |
Model Benchmark
|
🔬 Background
This mostly resolves #1235. I'm not sure how to proceed with plots for the conformal class since it has a different implementation.
🔮 Key changes
Adds argument that allows users to select a static display type. If this is specified, it sets the backend to plotly and then displays the plot in the specified type. The interactive plot is still returned from the function call.
📋 Review Checklist
Please make sure to follow our best practices in the Contributing guidelines.