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

Fix bug in pair plot axis handling #1985

Merged
merged 8 commits into from
Mar 1, 2022
Merged

Conversation

wm1995
Copy link
Contributor

@wm1995 wm1995 commented Feb 21, 2022

Description

This is an attempt at fixing #1981, in which the subplot axes do not have the same range by default but also lack unique labels. In doing so, I hope to correct some of the counterintuitive behaviour around passing "sharex" or "sharey" as backend parameters. I've written a fix, and am in the process of adding unit tests for said fix and unit tests for the fix.

Update (23/02/22):
I generated an example plot of the marginals, based on the plotpair example gallery code, to illustrate that the marginal axes now fall in the correct place.
example

Cf. the old behaviour, where the marginal plot axis ticks are not aligned with the plots beneath them, but the labels are also missing.

broken

Checklist

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

(I removed the "new features" documentation from the list because this isn't a new feature, just a bug fix to correct unexpected behaviour)

This commit fixes some of the issues in the current implementation of
plot_pair, making it possible to share axes between subplots in
plot_pair and allowing the user to make changes to the tick labels if
they so desire.
@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #1985 (b6fcefa) into main (9d74000) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1985   +/-   ##
=======================================
  Coverage   90.83%   90.84%           
=======================================
  Files         114      114           
  Lines       12311    12321   +10     
=======================================
+ Hits        11183    11193   +10     
  Misses       1128     1128           
Impacted Files Coverage Δ
arviz/plots/backends/matplotlib/pairplot.py 92.34% <100.00%> (+0.41%) ⬆️
arviz/plots/densityplot.py 91.07% <0.00%> (ø)

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 9d74000...b6fcefa. Read the comment docs.

This commit rewrites the code to reduce the number of new lines of code
needed, and makes the code more similar to the original code. This
(hopefully) has the added benefit of not changing test coverage.
@wm1995 wm1995 marked this pull request as ready for review February 23, 2022 16:11
@wm1995
Copy link
Contributor Author

wm1995 commented Feb 23, 2022

Right, I think this PR is ready for review now. The test I put together isn't strictly necessary, I think, but it is useful to make sure that all the axes you'd expect to be shared are indeed shared.

@wm1995
Copy link
Contributor Author

wm1995 commented Feb 23, 2022

Another example of the fix introduced by this PR is in the plot_pair_point_estimate example - cf. the original and the updated pages in the docs - the point estimates now actually line up in the marginal distribution and the joint distribution plots.

@wm1995 wm1995 changed the title [WIP] Fix bug in pair plot axis handling Fix bug in pair plot axis handling Feb 23, 2022
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement

arviz/plots/backends/matplotlib/pairplot.py Outdated Show resolved Hide resolved
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I left two minor comments, thank you so much for your contribution and patience while we discussed the approach to implement.

arviz/plots/backends/matplotlib/pairplot.py Outdated Show resolved Hide resolved
wm1995 and others added 2 commits March 1, 2022 21:43
Co-authored-by: Oriol Abril-Pla <oriol.abril.pla@gmail.com>
@wm1995
Copy link
Contributor Author

wm1995 commented Mar 1, 2022

I've made the requested changes, will just make sure that the CI runs alright and then if all is OK I'll leave it with you. Thanks for your help in putting this together and for being so responsive!

@OriolAbril OriolAbril linked an issue Mar 1, 2022 that may be closed by this pull request
@OriolAbril OriolAbril merged commit 0fbd405 into arviz-devs:main Mar 1, 2022
@wm1995 wm1995 deleted the fix-plot-pair branch March 1, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plot_pair marginals have inconsistent axes
2 participants