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

Add plot_compare #77

Merged
merged 21 commits into from
Aug 30, 2024
Merged

Add plot_compare #77

merged 21 commits into from
Aug 30, 2024

Conversation

aloctavodia
Copy link
Contributor

@aloctavodia aloctavodia commented Aug 2, 2024

closes #76

There are two major points I want to discuss for this PR (or we can split the discussion into more than one PR).

  • Is this the correct approach to implement this plot? I mean I am following the proper "new arviz logic"?
  • Modifications relative to the legacy plot_compare

about the second point I would like to simplify the plot and only show the ELPD point estimates + SE. Then we will get rid of the arguments insample_dev, plot_standard_error, plot_ic_diff, order_by_rank, legend. The main reason for this is that these are the most useful elements for the plot and this is how most people use and interpret this plot (including the equivalent plot in the Stan world).

I also would like to add two new arguments (already in the docstring), that I think could help make the plot easier to interpret.

  • similar_band : If True, a band is drawn to indicate models with similar predictive performance to the best model. (essentially 4 units from the point estimate of the best model) see figure below. I think this should be True by default
  • relative_scale : If True scale the ELPD values relative to the best model, so best model will be 0 and all the rest will be below 0 in the log-scale. Not sure if this should be true or false.

Captura desde 2024-08-02 11-23-08

Update

This is how it looks like now in

matplotlib
output_matplotlib

plotly (the band is missing from plotly, not sure why yet)
output_plotly

bokeh
output_bokeh

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 85.29412% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.71%. Comparing base (60aac9a) to head (6742f7a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/arviz_plots/plots/compareplot.py 90.32% 6 Missing ⚠️
src/arviz_plots/plot_collection.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   83.84%   84.71%   +0.86%     
==========================================
  Files          20       21       +1     
  Lines        2229     2336     +107     
==========================================
+ Hits         1869     1979     +110     
+ Misses        360      357       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OriolAbril
Copy link
Member

I really like the proposed changes. Completely agree on similar_band=True as default, not sure about relative, it is true we don't care about the absolute value, but I expect people to worry about the best one being exactly 0. We could try having it on and if we see people reporting back and getting confused we can turn it off

@aloctavodia aloctavodia requested a review from OriolAbril August 5, 2024 14:35
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

looks great, my main comment is using plot_kwargs and setdefault so it is more similar to other plots and offers similar felxibility

docs/source/gallery/model_comparison/plot_compare.py Outdated Show resolved Hide resolved
docs/source/gallery/model_comparison/plot_compare.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

General comment about this, I am still working on the example gallery and automating parts of the content that won't need our input. For example, instead of manually listing examples using the same function, I am trying to add a directive that generates a grid of elements (inspired from sphinx-gallery extension)

What parts do you think should not be part of your input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand your question.

Copy link
Member

Choose a reason for hiding this comment

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

I removed the (gallery_xyz)= for exmple, which is now automated, left only title, description and seealso box as user input now. But maybe we can improve that further

src/arviz_plots/plots/compareplot.py Outdated Show resolved Hide resolved
src/arviz_plots/plots/compareplot.py Outdated Show resolved Hide resolved
src/arviz_plots/plots/compareplot.py Outdated Show resolved Hide resolved
src/arviz_plots/plots/compareplot.py Outdated Show resolved Hide resolved
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

tried fixing the example for now, skimmed the code. It looks good, and uses the walrus! I had actually forgotten it existed 🤣 but I love it

Comment on lines +114 to +115
if isinstance(target, np.ndarray):
target = target.tolist()
Copy link
Contributor Author

@aloctavodia aloctavodia Aug 29, 2024

Choose a reason for hiding this comment

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

for the none backend there are many "target.append" instances (see here). But create_plotting_grid for none backend returns an array. These lines "fix" the issue without changing the current behavior on none backend, but maybe we should...

Copy link
Member

Choose a reason for hiding this comment

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

It generally is an array of empty lists, so then .map subsets the array and target ends up being the list that represents the desired subplot. It is very probably broken for (1,1) "grid" though so we get an empty array instead of an array with a single empty list 🤔

@aloctavodia aloctavodia merged commit f4a39af into main Aug 30, 2024
4 checks passed
@aloctavodia aloctavodia deleted the compare_plot branch August 30, 2024 13:07
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.

Add plot_compare
3 participants