Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix xlabels and ax argument in plot_elpd #1601

Merged
merged 3 commits into from
Mar 26, 2021
Merged

Fix xlabels and ax argument in plot_elpd #1601

merged 3 commits into from
Mar 26, 2021

Conversation

agustinaarroyuelo
Copy link
Contributor

@agustinaarroyuelo agustinaarroyuelo commented Mar 5, 2021

Description

Fix xlabels and ax argument in plot_elpd for numvars > 2.

elpd+2numvars

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@@ -179,7 +179,8 @@ def plot_elpd(
"{} - {}".format(models[i], models[j + 1]), fontsize=titlesize, wrap=True
)
if xlabels:
set_xticklabels(ax[-1, -1], coord_labels)
for i in range(len(ax)):
set_xticklabels(ax[-1, :][i], coord_labels)
Copy link
Member

Choose a reason for hiding this comment

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

I am not completely sure about what is causing the test error. Matplotlib locators do take steps as an argument, and there has been no breaking change on that. The only thing that would make sense would be for the loop to be over the last column instead of being over the bottom row so that set_xticklabels is trying to format an axis that has been set to off and thus has no locator. But then you would not have been able to generate the example image you shared, so I am completely at a loss here, no idea what is happening.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need to loop over the last row, the x axis is shared and should have the same labels for all plots automatically I think 🤔

@agustinaarroyuelo agustinaarroyuelo changed the title Fix xlabels and ax argument in plot_elpd [WIP]Fix xlabels and ax argument in plot_elpd Mar 7, 2021
@agustinaarroyuelo agustinaarroyuelo changed the title [WIP]Fix xlabels and ax argument in plot_elpd Fix xlabels and ax argument in plot_elpd Mar 26, 2021
@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #1601 (644e7ef) into main (a9917a2) will increase coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head 644e7ef differs from pull request most recent head 492dd90. Consider uploading reports for the commit 492dd90 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1601   +/-   ##
=======================================
  Coverage   90.92%   90.92%           
=======================================
  Files         108      108           
  Lines       11665    11666    +1     
=======================================
+ Hits        10606    10607    +1     
  Misses       1059     1059           
Impacted Files Coverage Δ
arviz/plots/backends/matplotlib/elpdplot.py 95.04% <66.66%> (+0.04%) ⬆️

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 a9917a2...492dd90. Read the comment docs.

@ahartikainen ahartikainen merged commit 8a1fd2c into arviz-devs:main Mar 26, 2021
@agustinaarroyuelo agustinaarroyuelo deleted the elpdplot_fix branch March 26, 2021 17:06
utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* fix ax argument for numvars > 2 and xlabels

* update changelog

* fix xlabels
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants