-
-
Notifications
You must be signed in to change notification settings - Fork 420
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
Changes from 10 commits
876dcc6
9721102
260e2e7
fde9f77
7c2b569
079cf5e
2553ad5
49ce2fe
62a7a67
ee8c9d1
ab06cd2
ae55486
ebe83e5
7faa75b
d0d7a30
d5d805b
4864849
ed73a13
e7f6a97
f596483
c59c6fe
bf376f1
d54e336
9911abc
fcefcfd
3a63dbe
a5a6060
bf598b9
92f9947
56fc92c
9a2a1f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
@@ -134,6 +135,22 @@ def plot_dist_comparison( | |
for data, var in zip(datasets, var_names) | ||
] | ||
|
||
total_plots = sum( | ||
sum(1 for _ in xarray_var_iter(data, var_names=var, combined=True)) | ||
for data, var in zip(datasets, var_names) | ||
) | ||
maxplots = sum(len(splot) for splot in dc_plotters) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
if total_plots > rcParams["plot.max_subplots"]: | ||
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=maxplots, len_plotters=total_plots), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be updated, not sure what is the best way to do so, but somehow. Here are the things that need to somehow be changed:
|
||
UserWarning, | ||
) | ||
for i in range(0, len(dc_plotters)): | ||
dc_plotters[i] = dc_plotters[i][:len_plots] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not necessary because the inner lists are truncated at creation, this would have no effect. |
||
|
||
nvars = len(dc_plotters[0]) | ||
ngroups = len(groups) | ||
|
||
|
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.
Here similar comment as for next line, the nested loop is not needed and is actually incorrect. It should be something like:
Also as we only want to count, we can use
sel
instead of usingvar
, we don't need to subset the data to get the values in each position to count how many positions there are.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.
Okay thank you for this explanation it was really useful and sorry for all the trouble caused, it's my first time contributing.
So what I have understood and already done (no commit yet) is that I have compared the
total_plots
which are the desired plots by the user with thercParams["plot.max_subplots"]
. Iftotal_plots
are greater than this then the warning is popping up and it does not truncate the plots because they are already truncated at the creation. I also used themaxplots
only at the description of the warning. I've also included a test case.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.
Sounds great, and don't worry about the discussion and back-and-forth comments, I think they were useful to all of us.