Development/plot dist comparison warning #1688

alexisperakis · 2021-05-06T13:25:24Z

Description

Checklist

Follows official PR format
Includes new or updated tests to cover the new feature
Code style correct (follows pylint and black guidelines)
Changes are listed in changelog

Issue: #1680

if statement added code to warn users if the subplots are over the limit. Notice that in the comparison, the length of the list should be increased by one:len(dc_plotters) + 1
There is a test case at the corresponding test file, with max_sublots set to 6.

Let me know if there is any issue regarding my work. Thank you for the help.

…t_matplotlib.py

OriolAbril

The .DS_Store files were added to the PR, they should be removed and probably have .gitignore ignore them from now on.

OriolAbril · 2021-05-06T15:18:16Z

arviz/plots/distcomparisonplot.py

@@ -130,9 +131,17 @@ def plot_dist_comparison(
    len_plots = rcParams["plot.max_subplots"] // (len(groups) + 1)
    len_plots = len_plots if len_plots else 1
    dc_plotters = [
-        list(xarray_var_iter(data, var_names=var, combined=True))[:len_plots]
+        list(xarray_var_iter(data, var_names=var, combined=True)) [:len_plots]


It looks like it is being truncated twice now, so I think the warning will always be triggered. Can you check locally in a few cases in addition to the test?

Yes, I will look into it

Should I alter the .gitignore?

Yes, adding things to the gitignore is fine, removing them is not

OriolAbril

there is still one ds store left

OriolAbril · 2021-05-07T15:58:09Z

arviz/plots/distcomparisonplot.py

        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]


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.

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]

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

you can plot exactly that number of subplots

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.

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

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?

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.

codecov · 2021-05-09T22:21:19Z

Codecov Report

Merging #1688 (fcefcfd) into main (7ebedd2) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head fcefcfd differs from pull request most recent head 92f9947. Consider uploading reports for the commit 92f9947 to get more accurate results

@@            Coverage Diff             @@
##             main    #1688      +/-   ##
==========================================
+ Coverage   90.86%   90.90%   +0.04%     
==========================================
  Files         108      108              
  Lines       11818    11767      -51     
==========================================
- Hits        10738    10697      -41     
+ Misses       1080     1070      -10

Impacted Files	Coverage Δ
arviz/plots/distcomparisonplot.py	`93.75% <100.00%> (+0.72%)`	⬆️
arviz/data/base.py	`97.87% <0.00%> (-0.35%)`	⬇️
arviz/data/io_pystan.py	`96.08% <0.00%> (+<0.01%)`	⬆️
arviz/data/io_cmdstanpy.py	`96.50% <0.00%> (+1.06%)`	⬆️
arviz/data/io_cmdstan.py	`90.95% <0.00%> (+1.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ebedd2...92f9947. Read the comment docs.

…t_matplotlib.py

OriolAbril

Added some comments, I think it will also be helpful to explain a bit better what the plotting function does to avoid further confusion.

plot_dist_comparison allows comparing distributions and can either compare 2 distributions between them or 3 of them at the same time. This is achieved by having a subplot per distribution and an extra plot where all 2 or 3 are overlayed together. Therefore, subplots are defined on a variable basis first, but each variable has 3-4 subplots, so subplots are added by blocks, not independently. In the case of comparing 2 dists, the look of the plot is the following:

====
(var1, dist1)  |  (var1, dist2)
----
  (var1, dist1+dist2)
====
(var2, dist1)  |  (var2, dist2)
----
  (var2, dist1+dist2)
====
...

The example in the docs has a single variable: https://arviz-devs.github.io/arviz/api/generated/arviz.plot_dist_comparison.html.

dc_plotters is a nested list of length # of distributions to compare, each of the lists within has length # of variables to plot. As it makes no sense to compare distributions yet plot one of them and not the one it should be compared with, we can't cut the number of plots by cutting dc_plotters we need to cut the number of variables (like we are currently doing).

len_plots takes this block structure into account to translate the number of subplots to number of varibles so we can use that value to cut the inner list of variables. Most of the new changes do not consider the extra overlayed plot and therefore would return incorrect results.

OriolAbril · 2021-05-11T08:03:25Z

arviz/plots/distcomparisonplot.py

+        for i in range(0, len(dc_plotters)):
+             dc_plotters[i] = dc_plotters[i][:len_plots]


This is not necessary because the inner lists are truncated at creation, this would have no effect.

OriolAbril · 2021-05-11T08:12:28Z

arviz/plots/distcomparisonplot.py

+        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)


maxplots as defined here is not necessarily equal to the rcParam, here plots are done in blocks of 3-4 subplots, so this will be equal or lower than the rcParam. To avoid confusion, I think the warning should use the rcParam first, then in the second occurrence it can be maxplots. It is not necessary nor correct to loop over dc_plotters though, it should be:

maxplots = len(dc_plotters[0]) * (len(groups) + 1)

OriolAbril · 2021-05-11T08:15:33Z

arviz/plots/distcomparisonplot.py

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


Here similar comment as for next line, the nested loop is not needed and is actually incorrect. It should be something like:

total_plots = sum(1 for _ in xarray_sel_iter(datasets[0], var_names=var_names[0], combined=True))

Also as we only want to count, we can use sel instead of using var, we don't need to subset the data to get the values in each position to count how many positions there are.

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 the rcParams["plot.max_subplots"]. If total_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 the maxplots only at the description of the warning. I've also included a test case.

Sounds great, and don't worry about the discussion and back-and-forth comments, I think they were useful to all of us.

OriolAbril · 2021-05-13T19:14:26Z

arviz/plots/distcomparisonplot.py

+    total_plots = sum(
+        1 for _ in xarray_sel_iter(datasets[0], var_names=var_names[0], combined=True)
+    )


I forgot to multiply in the sample code. This counts the number of variables which needs to be multiplied by (len(groups) + 1) to be equal to the total number of subplots that would be needed if we weren't truncating the lists of plotters (also read the next comment about the warning message).

OriolAbril · 2021-05-13T19:19:34Z

arviz/plots/distcomparisonplot.py

+            "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),


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:

smaller than the number of variables to plot does not work here. In most plots the number of subplots is the same as the number of variables, but here each variable has 3-4 subplots. Maybe using subplots instead of variable is enough?

rcParams['plot.max_subplots'] ({max_plots}) precisely because of this 3-4 factor, max_plots is not necessarily equal to the rcParam. I think it will be very confusing to users to set the rcParam to 7, then get a warning saying we'll only plot 6 subplots because you have set the rcParam to 6.

OriolAbril

I think this is good to merge, it's only missing being added to the changelog

…opment/plot_dist_comparison_warning Resolve conflicts

…st_comparison_warning

alexisperakis added 5 commits May 5, 2021 21:31

Add warning status for plot_dist_comparison method [WIP]

876dcc6

Changes to the warning

9721102

Change comparison variable for warning over the limit subplots

260e2e7

Fix a typo at distcomparisonplot.py, fix axes.shape(2, 3) at test_plo…

fde9f77

…t_matplotlib.py

Add + 1 in the if statement of the warnings

7c2b569

OriolAbril reviewed May 6, 2021

View reviewed changes

alexisperakis changed the title ~~Development/plot dist comparison warning~~ [WIP] Development/plot dist comparison warning May 6, 2021

DElete .DS_Store, alter .gitignore

079cf5e

OriolAbril reviewed May 7, 2021

View reviewed changes

Delete .DS_Store

2553ad5

alexisperakis added 14 commits May 10, 2021 02:10

Fix the counting of the elements of each sublist

49ce2fe

Change total subplots to compare with len plots

62a7a67

Nested list truncated arviz-devs#1688

ee8c9d1

Add warning status for plot_dist_comparison method [WIP]

ab06cd2

Changes to the warning

ae55486

Change comparison variable for warning over the limit subplots

ebe83e5

Fix a typo at distcomparisonplot.py, fix axes.shape(2, 3) at test_plo…

7faa75b

…t_matplotlib.py

Add + 1 in the if statement of the warnings

d0d7a30

DElete .DS_Store, alter .gitignore

d5d805b

Delete .DS_Store

4864849

Fix the counting of the elements of each sublist

ed73a13

Change total subplots to compare with len plots

e7f6a97

Nested list truncated arviz-devs#1688

f596483

Fix formating

c59c6fe

OriolAbril reviewed May 11, 2021

View reviewed changes

alexisperakis added 3 commits May 12, 2021 15:27

Resolve conflicts

bf376f1

Correct total subplots counting

d54e336

Black reformation

9911abc

OriolAbril reviewed May 13, 2021

View reviewed changes

Fix warning message

fcefcfd

OriolAbril approved these changes May 16, 2021

View reviewed changes

alexisperakis added 2 commits May 17, 2021 00:39

Add change to changelog arviz-devs#1688

3a63dbe

Fix a typo in changelog

a5a6060

OriolAbril approved these changes May 17, 2021

View reviewed changes

alexisperakis added 2 commits May 18, 2021 16:52

Fix git conflicts

bf598b9

Merge branch 'main' of https://github.com/arviz-devs/arviz into devel…

92f9947

…opment/plot_dist_comparison_warning Resolve conflicts

alexisperakis changed the title ~~[WIP] Development/plot dist comparison warning~~ Development/plot dist comparison warning Jun 27, 2021

OriolAbril added 2 commits March 4, 2022 22:58

Merge remote-tracking branch 'upstream/main' into development/plot_di…

56fc92c

…st_comparison_warning

fix changelog

9a2a1f1

OriolAbril merged commit cd256c4 into arviz-devs:main Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Development/plot dist comparison warning #1688

Development/plot dist comparison warning #1688

alexisperakis commented May 6, 2021 •

edited

Loading

OriolAbril left a comment

OriolAbril May 6, 2021

alexisperakis May 6, 2021

alexisperakis May 6, 2021

OriolAbril May 6, 2021

OriolAbril left a comment

OriolAbril May 7, 2021

alexisperakis May 9, 2021 •

edited

Loading

ColCarroll May 10, 2021

ColCarroll May 10, 2021

alexisperakis May 10, 2021

OriolAbril May 10, 2021

codecov bot commented May 9, 2021 •

edited

Loading

OriolAbril left a comment

OriolAbril May 11, 2021

OriolAbril May 11, 2021

OriolAbril May 11, 2021

alexisperakis May 13, 2021

OriolAbril May 13, 2021

OriolAbril May 13, 2021

OriolAbril May 13, 2021

OriolAbril left a comment

		for i in range(0, len(dc_plotters)):
		dc_plotters[i] = dc_plotters[i][:len_plots]

Development/plot dist comparison warning #1688

Development/plot dist comparison warning #1688

Conversation

alexisperakis commented May 6, 2021 • edited Loading

Description

Checklist

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexisperakis May 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 9, 2021 • edited Loading

Codecov Report

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

alexisperakis commented May 6, 2021 •

edited

Loading

alexisperakis May 9, 2021 •

edited

Loading

codecov bot commented May 9, 2021 •

edited

Loading