Add support for global figure titles#414
Conversation
|
Overall lgtm. We should update one or two examples in the gallery to show this new feature. Two candidates are: https://python.arviz.org/projects/plots/en/latest/gallery/plot_dist_kde.html (suggested title Posterior of mu per chain) I suggested these two examples because it should be easy to spot that the figure has a title from the miniatures, but other could work as well. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
==========================================
- Coverage 85.33% 85.19% -0.15%
==========================================
Files 59 59
Lines 6862 6925 +63
==========================================
+ Hits 5856 5900 +44
- Misses 1006 1025 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@aloctavodia Thanks for the suggestions! I've added figure titles to three gallery examples:
The titles should be clearly visible in the gallery thumbnails. Let me know if you'd like any adjustments! Gallery examples added! The ReadTheDocs error looks unrelated to these changes - happy to wait while other updates are being sorted out. Let me know if there's anything else you need from me! |
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
I haven't looked into the tests yet but my guess is we'll want to divide them between global plotcollection things (which might not even be parametrized with backend) and backend specific tests for the function that interfaces with the plotting backend and sets the title
|
@Shlokpalrecha if you rebase on |
|
Thanks for the detailed review! I see what you mean about the architecture- moving the backend-specific title code into each backend's core.py makes much more sense for maintainability. I'll work on:
I'll have these changes ready soon. Thanks for the clear guidance |
Adds figure_title parameter to PlotCollection.grid() and PlotCollection.wrap() methods, plus an add_title() method following the add_legend() pattern. Supported backends: - matplotlib: uses fig.suptitle() - plotly: uses fig.update_layout(title=...) - bokeh: uses Div element above the grid Closes arviz-devs#370
- Add descriptive title to plot_dist_kde showing KDE by chain - Add title to plot_ppc_rootogram for rugby model rootogram - Add title to plot_trace for MCMC diagnostic visualization Demonstrates the new figure_title feature across different plot types.
Moved title functionality from PlotCollection into backend-specific set_figure_title() functions in each backend's core.py. This follows the same pattern as other backend operations like show() and savefig(). Changes: - Created set_figure_title() in matplotlib, plotly, and bokeh backends - Updated add_title() to call backend functions instead of inline logic - Removed figure_title parameter from grid() and wrap() - users should call add_title() instead - Updated gallery examples to use add_title() method - Reorganized tests to separate PlotCollection tests from backend tests
Shlokpalrecha
force-pushed
the
add-figure-title
branch
from
5178b0f to
0ea5b1a
Compare
|
Thanks for the detailed feedback @OriolAbril! I've refactored everything as you suggested: Created set_figure_title() functions in each backend's core.py |
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
This is starting to look good, thanks! I have still left some higher level comments but some are already nits and implementation details.
OriolAbril
changed the title
|
Hey! Thanks for the detailed review, just pushed all the changes. Switched to the Everything's working on my end. Let me know if I missed anything or if there's anything else you'd like me to adjust. |
- Add @expand_aesthetic_aliases decorator to all backends (except none) - Use unset defaults instead of None for color/size parameters - Use _filter_kwargs helper for consistent arg handling - Add width: auto to bokeh Div for proper centering - Add ALLOW_KWARGS check to none backend - Use B1 as default color in add_title - Rename artist_kws to kwargs in add_title for user clarity - Filter size=None to avoid passing it to backend
|
Addressed all feedback- ready for re-review, thankyou |
|
Hi @OriolAbril, just a gentle ping to say I’ve pushed updates addressing all the review comments and requested a re-review. No rush at all, just whenever you have time. Thanks again for the detailed feedback. |
| The title text object. | ||
| """ | ||
| kwargs = {"color": color, "fontsize": size} | ||
| title_obj = figure.suptitle(text, **_filter_kwargs(kwargs, None, artist_kws)) |
There was a problem hiding this comment.
| title_obj = figure.suptitle(text, **_filter_kwargs(kwargs, None, artist_kws)) | |
| title_obj = figure.suptitle(text, **_filter_kwargs(kwargs, Text, artist_kws)) |
| title_kwargs = {"text": text, "x": 0.5, "xanchor": "center"} | ||
| title_kwargs = _filter_kwargs( | ||
| {"font_color": color, "font_size": size}, {**title_kwargs, **artist_kws} | ||
| ) |
There was a problem hiding this comment.
| title_kwargs = {"text": text, "x": 0.5, "xanchor": "center"} | |
| title_kwargs = _filter_kwargs( | |
| {"font_color": color, "font_size": size}, {**title_kwargs, **artist_kws} | |
| ) | |
| title_kwargs = _filter_kwargs( | |
| {"font_color": color, "font_size": size, "text": text}, {"x": 0.5, "xanchor": "center"} | artist_kws | |
| ) |
The number of lines doesn't really matter, the reasoning for the changes is being as clear as possible on how we want every argument to behave like. color, font size or text should only be passed through the dedicated arguments. If I use color="red", font_color="blue" the result should be red. However, if I want a left aligned title or any other funky looking title x and xanchor can be modified through artist_kws.
note: the 2nd positional argument is named text, so in practice artist_kws can't really have a text key to overwrite the one in the original title_kwargs but I think separating the arguments depending on behaviour reduced the cognitive load in understanding what the function is doing.
github_comment.txt
Outdated
tests/test_figure_title.py
Outdated
| class TestPlotCollectionTitleMethod: | ||
| """Test PlotCollection.add_title() method (not backend-specific).""" | ||
|
|
||
| def test_add_title_method_exists(self, dataset): |
There was a problem hiding this comment.
we don't need this test. It is a normal method not something dynamically added or other complicated architectures, and we have tests for it, so if someone deletes it by mistake tests will fail.
tests/test_figure_title.py
Outdated
| pc = PlotCollection.grid( | ||
| dataset, | ||
| cols=["__variable__"], | ||
| backend="matplotlib", |
There was a problem hiding this comment.
backend agnostic tests should use the none backend. Given we don't care about what gets rendered, only we an call the method and it modifies pc.viz there is no need to use a proper plotting backend, it only slows down the test suite.
tests/test_figure_title.py
Outdated
| @pytest.mark.parametrize("backend", ["matplotlib", "bokeh", "plotly"]) | ||
| @pytest.mark.usefixtures("clean_plots") | ||
| @pytest.mark.usefixtures("check_skips") | ||
| class TestBackendSetFigureTitle: |
There was a problem hiding this comment.
these tests should go in the backend specific files (this depends on #409 which I am hoping to finish in 1-2 days). You'll see there the pattern is generally one test using the backend function with only required arguments, and one test using arguments like color and size. If we identify other cases that need dedicated tests then we add more.
If you want to avoid getting too many git conflicts you can start the tests here even if they don't run and I can move them and fix conflicts when I merge the other PR.
tests/test_figure_title.py
Outdated
| class TestBackendSetFigureTitle: | ||
| """Test backend-specific set_figure_title functions.""" | ||
|
|
||
| def test_set_figure_title_basic(self, dataset, backend): |
There was a problem hiding this comment.
this can be the base for the first test I mention in the other comment but using the backend functions directly.
tests/test_figure_title.py
Outdated
| def test_set_figure_title_with_color(self, dataset, backend): | ||
| """Test set_figure_title with color parameter.""" | ||
| pc = PlotCollection.grid( | ||
| dataset, | ||
| cols=["__variable__"], | ||
| backend=backend, | ||
| ) | ||
| pc.add_title("Colored Title", color="red") | ||
| assert "figure_title" in pc.viz | ||
|
|
||
| def test_set_figure_title_with_size(self, dataset, backend): | ||
| """Test set_figure_title with size parameter.""" | ||
| pc = PlotCollection.grid( | ||
| dataset, | ||
| cols=["__variable__"], | ||
| backend=backend, | ||
| ) | ||
| # Use string size for bokeh compatibility | ||
| pc.add_title("Sized Title", size="16px") | ||
| assert "figure_title" in pc.viz |
There was a problem hiding this comment.
These two should be combined into a single one. Also note the main goal of this test using arguments is ensuring they are propagated properly to the backend. So the assert figure_title in pc.viz doesn't add anything (and there won't be a plot_collection in the test either) and what needs to be checked is the backend object has the right properties
tests/test_figure_title.py
Outdated
| pc.add_title("Sized Title", size="16px") | ||
| assert "figure_title" in pc.viz | ||
|
|
||
| def test_backward_compatibility_no_title(self, dataset, backend): |
tests/test_figure_title.py
Outdated
| """Verify that a title was added (basic check).""" | ||
| if backend == "matplotlib": | ||
| assert hasattr(fig, "_suptitle") | ||
| assert fig._suptitle is not None |
There was a problem hiding this comment.
for when adapting the test to backend specific. if there is no alternative we can use private attributes but if we can it is best to avoid them as any minor change in matplotlib could break our tests. For this I think a combination of get_suptitle and fig.texts will allow us to check everything we care about.
- Use Text class for matplotlib _filter_kwargs validation - Restructure plotly _filter_kwargs with dict union operator for clear argument precedence - Move title tests to test_plot_collection.py - Use 'none' backend for agnostic tests - Remove github_comment.txt
|
Hey! @OriolAbril Just pushed the changes from your latest review. Switched to the Text class for matplotlib's filter validation, restructured the plotly kwargs with the union operator for clearer precedence, and moved the tests over to test_plot_collection.py with the none backend. Should be good now. Let me know if you need anything else. |
|
Thanks @Shlokpalrecha! That was a very big PR to get started |
4 participants
Closes #370
Summary
Adds support for global figure titles across all backends, as discussed in the issue.
Changes
add_title()method toPlotCollection(following theadd_legend()pattern as suggested)figure_titleparameter toPlotCollection.grid()andPlotCollection.wrap()for conveniencefig.suptitle()fig.update_layout(title=...)Divelement above the gridUsage