-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Proposal: BBoxs should be a a ring of sets #17115
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
base: main
Are you sure you want to change the base?
Conversation
Obviously WIP, as I don't think even the docs build as-is, and the new kwarg designed to be used to deprecate the old behavior is currently set to its "new" value by default. |
Is this supposed to say |
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.
I think this is a good idea - mathematically sound semantics is awesome - but I'm worried that the explanation is a lil too phd mathy. Like I think it's useful to include it for correctness, but the baseline/first explanations should be geared very general audience.
lib/matplotlib/transforms.py
Outdated
if not null_as_empty: | ||
cbook.warn_deprecated( | ||
3.4, message="Bboxs will soon change their behavior to " | ||
"correctly treat empty Bboxs as empty sets in unions and " |
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.
Struggling with this error message 'cause on the one hand like I know why you're writing it cause the operations are unions & intersections...and on the other it's meaningless w/o an understanding of basic set theory. What concretely/what input is triggering this error?"
A related point I had from my notes, but for which I never wrote anything: this may be a good opportunity to also check the return type of get_window_extent()/get_tightbbox(). I think the way these return a "null" bbox is not always very clear. I guess it's usually None, but that means one then needs to explicitly take that into account when unioning bboxes (e.g. for tight-layout or for savefig(bbox_inches="tight")). It would be much simpler if they indeed always returned a real bbox which can be properly unioned/intersected. |
c7df9f0
to
46a6025
Compare
Looking at this again, while I agree that requiring "oriented" bboxes may be best, perhaps an easier way out for now is to make |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
I think some form of this would definitely be good to have. |
PR Summary
As was discussed in #17090, the semantics of
Bbox.null
are very unexpected.While it is supposed to represent the empty set, it currently has pretty surprising behavior under intersection and union (which, in particular, does not correspond to the behavior of the empty set).
This is because union and intersection are currently implemented by comparing
xmin
andxmax
instead ofx0
andx1
(respectively,ymin
, ...etc.).This PR corrects the semantics of Bboxs so that they are a ring of sets, with a unique null set (
Bbox.null()
) as an additive identity and a unique multiplicative identity (Bbox.unbounded()
).This is a major change that fundamentally changes how Bbox's work, and I will need to do some digging to ensure it doesn't break anything internally. However, so far I don't see any substantive issues in the test suite, just a few cases where axes bounds relied on the previous buggy behavior, all look like simple fixes.
Implementation details
In order to change as little code as possible, and maintain keep any API change as unsurprising as possible, I kept the existing representation of the empty set,
Bbox([[inf, inf], [-inf, -inf]])
.In order to fix
intersection
andunion
, they should simply follow the well-established formulas:and
It is easy to check that the existing representation of the empty set (
Bbox.null
) produces the correct behavior in both of these formulas.However, there definitely can be produce pathological behavior if we allow "improper" intervals of the form$[b, a]$ for $b > a$ .
After experimenting for some time with the algebraic properties of these improper intervals, I concluded that
and
are simply not well defined in these cases. Our existing behavior is almost definitely a bug, for example, while the following intersection returns the (reasonable) reply of "None":
The "union" operator currently finds the Bbox by something equivalent to
update_from_data_xy(np.stack([bbox.get_points() for bbox in bboxes]), ignore=True)
:This is almost certainly not I would want to happen, as it means that the union of two empty Bboxs can create a Bbox that's not empty. But I could see how existing code might abuse this fact...
Deprecation schedule
Since the existing intersection and union results might be used by quite a bit of code, (even internally), I assume any deprecation schedule should be commensurately conservative.
The "3.4" in this PR is more of a placeholder in order to allow discussion than anything else.
PR Checklist