test: comprehensive tests for combine_plots (fixes #236)#438
Conversation
|
Thanks for the PR, we are a bit swamped right now so expect reviews to take at least several days. Also note that unit tests and hypothesis tests are generally welcome even if there is no specific issue (I'd have to review what issues we have open around that) but neither of these two things will close #236 |
Thank you for the feedback and for taking the time to respond! I understand the team is busy and I'm happy to wait for the review. Could you please guide me on what would be needed to close #236? I'd love to work on it and contribute meaningfully to the project. I'll also go through the issue in detail in the meantime. Thanks again for the warm response! |
- Remove 'none' backend (combine_plots calls render() which needs a real backend) - Remove no_artist_kwargs fixture (internal backend='none' calls need kwargs) - Replace plot_trace combos with plot_dist variants (plot_trace has internal grid dimensions that conflict with combine_plots) - Add xfail test documenting plot_forest incompatibility bug - Add tests for: column/row expand, custom names, sample_dims forwarding, invalid expand error, multiple plots, kwargs, PPC plots Ref: arviz-devs#236
jinukuntlaakhilakumargoud-web
changed the title
OriolAbril
left a comment
OriolAbril
left a comment
There was a problem hiding this comment.
I would include these tests in test_plots, within the TestPlots class with the exception of the test for the exclude error which should go elsewhere.
I am not completely sure where though, the main two options that come to mind are as a function in test_plots like the test_plot_dist_kind_auto function or in a new test_plot_errors file. Any opinions on that @aloctavodia ?
| class TestCombinePlots: | ||
| """Test suite for combine_plots (issue #236).""" | ||
|
|
||
| def test_combine_basic_column(self, datatree, backend): |
There was a problem hiding this comment.
this can be combined with the test for row below using a function specific pytest.mark.parametrize
| assert isinstance(pc, PlotCollection) | ||
| assert "figure" in pc.viz.data_vars | ||
|
|
||
| def test_combine_invalid_expand(self, datatree, backend): |
There was a problem hiding this comment.
we don't have many (or any) tests explicitly checking for errors but it would be nice. The main comment on this is we always make sure errors are raised as soon as possible, which in the vast majority of cases means before much of the processing and before any plotting happens. Therefore there is no point in parametrizing this test with backend. It should use "none" if possible, otherwise "matplotlib" explicitly and be outside the TestPlots class.
| def test_combine_multiple_plots(self, datatree, backend): | ||
| """Three plots can be combined in a single figure.""" | ||
| pc = combine_plots( | ||
| datatree, | ||
| plots=[ | ||
| (plot_dist, {}), | ||
| (plot_dist, {"kind": "ecdf"}), | ||
| (plot_dist, {"kind": "hist"}), | ||
| ], | ||
| backend=backend, | ||
| ) | ||
| assert isinstance(pc, PlotCollection) | ||
| assert "figure" in pc.viz.data_vars | ||
| assert pc.viz["plot"].sizes["column"] == 3 | ||
|
|
||
| def test_combine_with_kwargs(self, datatree, backend): | ||
| """Extra kwargs are forwarded to individual plot functions.""" | ||
| pc = combine_plots( | ||
| datatree, | ||
| plots=[ | ||
| (plot_dist, {"kind": "ecdf"}), | ||
| (plot_dist, {"kind": "hist"}), | ||
| ], | ||
| backend=backend, | ||
| ) | ||
| assert isinstance(pc, PlotCollection) | ||
| assert "figure" in pc.viz.data_vars |
There was a problem hiding this comment.
these two are redundant with the basic tests
| ), | ||
| strict=False, | ||
| ) | ||
| def test_combine_with_forest_xfail(self, datatree, backend): |
There was a problem hiding this comment.
I don't think it is worth adding xfail tests unless we have a very clear reason too. In this case it is very clear functions like plot_forest, plot_energy, plot_trace_dist won't work with combine_plots at a conceptual level.
| ``combine_plots`` internally renders child plots with ``backend='none'`` before | ||
| mapping them onto a real grid, so the outer backend must be a real plotting | ||
| backend (matplotlib, bokeh, plotly) and the ``no_artist_kwargs`` fixture must be | ||
| omitted. |
There was a problem hiding this comment.
I don't think we actually want either of these two things. It is pointless but we can "rerender" the plot with the none backend again and it should work. And if we are triggering the no_artist_kwargs check through the combine_plots testing that is definitely something to double check.
The issue is about user testing. We need people to, from the role of ArviZ users, play around with I will update the issue description to be clearer, this is one of the reasons the "good first issue" label can be misleading. This is an extremely easy issue for an ArviZ user as it doesn't require opening a PR or learning about the contributing workflow, but extremely difficult for someone who is not an ArviZ user (even if they are a pro at contributing, PRs, CI...) because it requires first becoming an ArviZ user, then working on the issue. |
2 participants
Summary
Added comprehensive test suite for
combine_plotswhich previously had zero tests. This addresses issue #236 ("User test and improve combine_plots").Tests added (9 total)
test_combine_basic_columnexpand="column"layouttest_combine_basic_rowexpand="row"layouttest_combine_custom_plot_namestest_combine_sample_dims_forwardedsample_dimsreaches child plotstest_combine_invalid_expandtest_combine_multiple_plotstest_combine_with_kwargstest_combine_ppc_plotsgroup="posterior_predictive"test_combine_with_forest_xfailplot_forestincompatibilityKey findings
sample_dimsforwarding - Already fixed in the current codebase (line 156 ofcombine.py)no_artist_kwargsincompatibility -combine_plotsinternally calls child plots withbackend="none", so theno_artist_kwargstest fixture cannot be usedplot_forest,plot_ridge,plot_ess, andplot_tracefail when used withcombine_plotsbecause their internal grid dimensions conflict withcombine_plots'scolumn/rowexpand dimension, causingbackend_from_object()to receive an xarray object instead of a plotting targetBug details
The
render()function incombine.pycallsbackend_from_object(target)which inspectstarget.__module__to determine the backend. When grid-based plots (forest, ridge, ess, trace) are combined, thePlotCollection.map()call may resolvetargetto an xarray DataArray instead of a matplotlib Axes, causing aValueError.This is documented via an
xfailtest (test_combine_with_forest_xfail).