-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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.
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( |
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.
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.
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 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.
b6d26c8
to
364d4a4
Compare
364d4a4
to
d834b01
Compare
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 test is a distilled out of matplotlib#16552.
This is mostly for the sake of third-party `Collection` subclasses that might have overridden `get_datalim`.
d834b01
to
e85ea1b
Compare
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.
Thanks!
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 theBbox
that's passed between those two functions, and uses it when updatingAxes.dataLim
.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
andpydocstyle<4
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).