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

Development/plot dist comparison warning #1688

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
876dcc6
Add warning status for plot_dist_comparison method [WIP]
alexisperakis May 5, 2021
9721102
Changes to the warning
alexisperakis May 5, 2021
260e2e7
Change comparison variable for warning over the limit subplots
alexisperakis May 5, 2021
fde9f77
Fix a typo at distcomparisonplot.py, fix axes.shape(2, 3) at test_plo…
alexisperakis May 5, 2021
7c2b569
Add + 1 in the if statement of the warnings
alexisperakis May 6, 2021
079cf5e
DElete .DS_Store, alter .gitignore
alexisperakis May 7, 2021
2553ad5
Delete .DS_Store
alexisperakis May 9, 2021
49ce2fe
Fix the counting of the elements of each sublist
alexisperakis May 9, 2021
62a7a67
Change total subplots to compare with len plots
alexisperakis May 10, 2021
ee8c9d1
Nested list truncated #1688
alexisperakis May 10, 2021
ab06cd2
Add warning status for plot_dist_comparison method [WIP]
alexisperakis May 5, 2021
ae55486
Changes to the warning
alexisperakis May 5, 2021
ebe83e5
Change comparison variable for warning over the limit subplots
alexisperakis May 5, 2021
7faa75b
Fix a typo at distcomparisonplot.py, fix axes.shape(2, 3) at test_plo…
alexisperakis May 5, 2021
d0d7a30
Add + 1 in the if statement of the warnings
alexisperakis May 6, 2021
d5d805b
DElete .DS_Store, alter .gitignore
alexisperakis May 7, 2021
4864849
Delete .DS_Store
alexisperakis May 9, 2021
ed73a13
Fix the counting of the elements of each sublist
alexisperakis May 9, 2021
e7f6a97
Change total subplots to compare with len plots
alexisperakis May 10, 2021
f596483
Nested list truncated #1688
alexisperakis May 10, 2021
c59c6fe
Fix formating
alexisperakis May 10, 2021
bf376f1
Resolve conflicts
alexisperakis May 12, 2021
d54e336
Correct total subplots counting
alexisperakis May 13, 2021
9911abc
Black reformation
alexisperakis May 13, 2021
fcefcfd
Fix warning message
alexisperakis May 15, 2021
3a63dbe
Add change to changelog #1688
alexisperakis May 16, 2021
a5a6060
Fix a typo in changelog
alexisperakis May 16, 2021
bf598b9
Fix git conflicts
alexisperakis May 18, 2021
92f9947
Merge branch 'main' of https://github.com/arviz-devs/arviz into devel…
alexisperakis May 24, 2021
56fc92c
Merge remote-tracking branch 'upstream/main' into development/plot_di…
OriolAbril Mar 4, 2022
9a2a1f1
fix changelog
OriolAbril Mar 4, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ saved_animations/
# mypy
.mypy_cache

# MacOS generated files
.DS_Store

# Stan file
doc/getting_started/*.stan

Expand Down
9 changes: 9 additions & 0 deletions arviz/plots/distcomparisonplot.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Density Comparison plot."""
import warnings
from ..labels import BaseLabeller
from ..rcparams import rcParams
from ..utils import _var_names, get_coords
Expand Down Expand Up @@ -133,6 +134,14 @@ def plot_dist_comparison(
list(xarray_var_iter(data, var_names=var, combined=True))[:len_plots]
for data, var in zip(datasets, var_names)
]
if len(dc_plotters) + 1 > len_plots:
warnings.warn(
"rcParams['plot.max_subplots'] ({max_plots}) is smaller than the number "
"of variables to plot ({len_plotters}), generating only {max_plots} "
"plots".format(max_plots=len_plots, len_plotters=len(dc_plotters)),
UserWarning,
)
dc_plotters = dc_plotters[:len_plots]
Copy link
Member

Choose a reason for hiding this comment

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

the xarray_var_iter list is still getting truncated when defining dc_plotters, and it should be truncated at that level but afterwards while printing the warning.

dc_plotters will have length of 2 or 3 always, as it is a list of lists, it's those sublists the ones that need to be truncated.

Copy link
Contributor Author

@alexisperakis alexisperakis May 9, 2021

Choose a reason for hiding this comment

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

Okay so I change the code by adding a counter, summing up the length of each list and then compare it with len_plots.
I also used a for loop to truncate the sublists. However, I am not sure about the tests and also do I have to truncate each sublist with the [:len_plots]

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is confusing since len_plots is different from rcParams["plot.max_subplots"], and it isn't obvious to me that

  1. you can plot exactly that number of subplots
  2. you won't plot more

I think

total_plots = sum(sum(1 for _ in xarray_var_iter(data, var_names=var, combined=True)) for data, var in zip(datasets, var_names))

gives you the number of "desired plots". I would check if that number is bigger than rcParams["plot.max_subplots"], and if so, issue a warning, and say you'll plot however many things are in dc_plotters (which i guess is sum(len(j) for j in dc_plotters)). The calculation on line 134 already verifies that you shouldn't have to truncate further (I think?), so you'd just have to issue a warning.

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, I think it is an accident that the test passes, and line 137 isn't the condition we care about at all!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I also thought that at line 134 dc_plotters = [ list(xarray_var_iter(data, var_names=var, combined=True))[:len_plots]dc_plotters is already truncated, so I think that it should stay as is.
However, do you think I should compare total_plots with rcParams["plot.max_subplots"] and not with len_plots which divides rcParams["plot.max_subplots"] with (len(groups) + 1), because it was mentioned that we want to cut dc_plotters to have at most len_plots?

Copy link
Member

Choose a reason for hiding this comment

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

alexisperakis you should disregard the comment in the issue (the one you linked) and focus on this comment (the beggining of this discussion). Sorry about the initial confusion, I did not look carefully enough at the code in the issue, but in general, if a newer comment contradicts an older one, pay atention to the new one only, at least in the parts where they contradict each other.

dc_plotters has length 2 or 3 always and it does not have to be truncated in any case. The nested lists within dc_plotters are the ones that need to be truncated to have at most len_plots.


nvars = len(dc_plotters[0])
ngroups = len(groups)
Expand Down
Binary file added arviz/tests/.DS_Store
Binary file not shown.
7 changes: 7 additions & 0 deletions arviz/tests/base_tests/test_plots_matplotlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,13 @@ def test_plot_trace_max_subplots_warning(models):
assert axes.shape == (3, 2)


def test_plot_dist_comparison_warning(models):
with pytest.warns(UserWarning):
with rc_context(rc={"plot.max_subplots": 6}):
axes = plot_dist_comparison(models.model_1)
assert axes.shape == (2, 3)


@pytest.mark.parametrize("kwargs", [{"var_names": ["mu", "tau"], "lines": [("hey", {}, [1])]}])
def test_plot_trace_invalid_varname_warning(models, kwargs):
with pytest.warns(UserWarning, match="valid var.+should be provided"):
Expand Down