Skip to content

Prevents axes limits from being resized by axes.fill_between #25712

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 28, 2023
Merged

Prevents axes limits from being resized by axes.fill_between #25712

merged 4 commits into from
Apr 28, 2023

Conversation

olinjohnson
Copy link
Contributor

@olinjohnson olinjohnson commented Apr 18, 2023 •
edited by jklymak
Loading

PR Summary

Closes #25682 by preventing Axes.fill_between() from automatically adjusting the axes limits when using axes coordinates. When axes coordinates are not in use and thus no transformation is passed to Axes.fill_between(), then adjusting the axes limits is necessary, but previously, in some cases, a transformation might have been in use and using axes coordinates could have caused the axes limits to be resized improperly.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@olinjohnson olinjohnson changed the title fixed issue #25682 Fixes #25682 - prevents axes limits from being unnecessarily resized Apr 18, 2023
github-actions[bot]
github-actions bot reviewed Apr 18, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@ksunden
Copy link
Member

ksunden commented Apr 18, 2023

This could probably use a test of this behavior, something like the code from #25682

I don't think it needs to be an image comparison test, just an assertion that the limits are the same before/after the fillbetween call

QuLogic
QuLogic reviewed Apr 18, 2023
View reviewed changes
lib/matplotlib/axes/_axes.py Outdated
Comment on lines 5413 to 5414
if "transform" not in kwargs:
self.update_datalim(pts, updatex=True, updatey=True)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably check the transform branches, similar to how lines do:

matplotlib/lib/matplotlib/axes/_base.py

Line 2354 in 690884f

updatex, updatey = line_trf.contains_branch_seperately(self.transData)

Copy link
Member

Choose a reason for hiding this comment

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

That may be a bit more complete, yeah... for reference I based the original suggestion on axline:

matplotlib/lib/matplotlib/axes/_axes.py

Lines 910 to 913 in b86ebba

if "transform" in kwargs:
# if a transform is passed (i.e. line points not in data space),
# data limits should not be adjusted.
datalim = []

Copy link
Contributor Author

@olinjohnson olinjohnson Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@QuLogic I'm not yet very well-versed with transforms - wondering if you could provide some guidance as to how this would work?

I will continue to look into this as well.

Copy link
Member

Choose a reason for hiding this comment

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

See #25324 for a similar application of the transform branches for update_datalim on vlines/hlines. (Though I don't know for sure that you also need to handle rectilinear specifically as is done in that PR; just the transform bit might be enough.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuLogic @ksunden I updated the PR to check transform branches - can you review this to make sure everything looks alright?

@jklymak jklymak changed the title Fixes #25682 - prevents axes limits from being unnecessarily resized Prevents axes limits from being resized by axes.fill_between Apr 19, 2023
@olinjohnson
Copy link
Contributor Author

olinjohnson commented Apr 20, 2023 •
edited
Loading

@ksunden I did a few basic image tests after initially making the changes:
Screen Shot 2023-04-20 at 7 34 44 PM
Screen Shot 2023-04-20 at 7 33 16 PM
As you can see, the example using the code from the documentation remains the same (as expected), and the example using the code for reproduction from #25712 returns the expected output after the change.

Additionally, I also tested with an assertion (per your recommendation) which verified that the axes limits remain the same before and after the fill_between call.

original_lims = (ax.get_xlim(), ax.get_ylim())

ax.fill_between(x, 0, 1, where=y > threshold2,
                color='green', alpha=0.5, transform=ax.get_xaxis_transform())

assert (ax.get_xlim(), ax.get_ylim()) == original_lims

@ksunden
Copy link
Member

ksunden commented Apr 21, 2023

@olinjohnson could you make this a unit test in lib/matplotlib/tests/test_axes.py?

https://matplotlib.org/stable/devel/testing.html#writing-a-simple-test

This will help us to ensure that the expected behavior is retained through future changes.

@olinjohnson
Copy link
Contributor Author

Yes! I added the unit test - sorry, should have done this before.

QuLogic
QuLogic approved these changes Apr 25, 2023
View reviewed changes
@olinjohnson olinjohnson requested a review from ksunden April 28, 2023 01:40
@olinjohnson @QuLogic
Update lib/matplotlib/axes/_axes.py
8c5305f 
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@ksunden ksunden merged commit 6be5be8 into matplotlib:main Apr 28, 2023
@ksunden
Copy link
Member

ksunden commented Apr 28, 2023

Thank you for your contribution, hope to hear from you again!

@QuLogic QuLogic added this to the v3.8.0 milestone Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@QuLogic QuLogic QuLogic approved these changes

@ksunden ksunden ksunden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.

[Bug]: fill_between{x} does not respect Axes transform
3 participants
@olinjohnson @ksunden @QuLogic