Skip to content

Propagate minpos from Collections to Axes.datalim #18642

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

Merged
merged 5 commits into from
Jan 5, 2021

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 3, 2020

PR Summary

This is an attempt to fix #16552. I'm not sure if it's the best change, API-wise, but seems to work without breaking anything. Possibly could do with an API change note.

Essentially, we have the right information to do log auto-scaling correct, but that's thrown away at the Collection.get_datalim/Axes.add_collection interface. This propagates that information onto the Bbox that's passed between those two functions, and uses it when updating Axes.dataLim.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic QuLogic added this to the v3.4.0 milestone Oct 3, 2020
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

this looks correct, but I'd need to spend more time with it to completely follow it. Can you add comments? Otherwise its pretty mysterious...

@@ -274,11 +274,11 @@ def get_datalim(self, transData):
# can properly have the axes limits set by their shape +
# offset. LineCollections that have no offsets can
# also use this algorithm (like streamplot).
result = mpath.get_path_collection_extents(
transform.get_affine(), paths, self.get_transforms(),
return mpath.get_path_collection_extents(
Copy link
Member

Choose a reason for hiding this comment

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

again, this could get some more explanation, given that there is a comment above, but this is clearly different. I'm not even sure I understand why you can change this like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The first argument is the master transform; instead of doing one transform when finding extents, and a second one after on the result, this does a combined transform from the get-go. This ensures that whatever is calculated for minpos is in the final coordinate space.

@QuLogic QuLogic force-pushed the collection-datalim branch from b6d26c8 to 364d4a4 Compare October 7, 2020 00:01
@QuLogic QuLogic force-pushed the collection-datalim branch from 364d4a4 to d834b01 Compare October 9, 2020 22:03
This is already calculated by the internal C++ code, but discarded at
the end of the Python function.
This ensures that autoscaling on log scales is correct.
This is mostly for the sake of third-party `Collection` subclasses that
might have overridden `get_datalim`.
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scatter autoscaling still has issues with log scaling and zero values
3 participants