-
-
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
Merged
OriolAbril
merged 31 commits into
arviz-devs:main
from
alexisperakis:development/plot_dist_comparison_warning
Mar 4, 2022
+28
−1
Merged
Changes from 7 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 9721102
Changes to the warning
alexisperakis 260e2e7
Change comparison variable for warning over the limit subplots
alexisperakis fde9f77
Fix a typo at distcomparisonplot.py, fix axes.shape(2, 3) at test_plo…
alexisperakis 7c2b569
Add + 1 in the if statement of the warnings
alexisperakis 079cf5e
DElete .DS_Store, alter .gitignore
alexisperakis 2553ad5
Delete .DS_Store
alexisperakis 49ce2fe
Fix the counting of the elements of each sublist
alexisperakis 62a7a67
Change total subplots to compare with len plots
alexisperakis ee8c9d1
Nested list truncated #1688
alexisperakis ab06cd2
Add warning status for plot_dist_comparison method [WIP]
alexisperakis ae55486
Changes to the warning
alexisperakis ebe83e5
Change comparison variable for warning over the limit subplots
alexisperakis 7faa75b
Fix a typo at distcomparisonplot.py, fix axes.shape(2, 3) at test_plo…
alexisperakis d0d7a30
Add + 1 in the if statement of the warnings
alexisperakis d5d805b
DElete .DS_Store, alter .gitignore
alexisperakis 4864849
Delete .DS_Store
alexisperakis ed73a13
Fix the counting of the elements of each sublist
alexisperakis e7f6a97
Change total subplots to compare with len plots
alexisperakis f596483
Nested list truncated #1688
alexisperakis c59c6fe
Fix formating
alexisperakis bf376f1
Resolve conflicts
alexisperakis d54e336
Correct total subplots counting
alexisperakis 9911abc
Black reformation
alexisperakis fcefcfd
Fix warning message
alexisperakis 3a63dbe
Add change to changelog #1688
alexisperakis a5a6060
Fix a typo in changelog
alexisperakis bf598b9
Fix git conflicts
alexisperakis 92f9947
Merge branch 'main' of https://github.com/arviz-devs/arviz into devel…
alexisperakis 56fc92c
Merge remote-tracking branch 'upstream/main' into development/plot_di…
OriolAbril 9a2a1f1
fix changelog
OriolAbril File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
the
xarray_var_iter
list is still getting truncated when definingdc_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.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 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]
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.
Yeah, this is confusing since
len_plots
is different fromrcParams["plot.max_subplots"]
, and it isn't obvious to me thatI think
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 indc_plotters
(which i guess issum(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.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.
Specifically, I think it is an accident that the test passes, and line 137 isn't the condition we care about at all!
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.
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
withrcParams["plot.max_subplots"]
and not withlen_plots
which dividesrcParams["plot.max_subplots"]
with(len(groups) + 1)
, because it was mentioned that we want to cutdc_plotters
to have at mostlen_plots
?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.
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 withindc_plotters
are the ones that need to be truncated to have at mostlen_plots
.