Skip to content

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brunobeltran
Copy link
Contributor

@brunobeltran brunobeltran commented Apr 13, 2020

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 and xmax instead of x0 and x1 (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 and union, 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":

>>> Bbox.intersection(Bbox([[2, 2], [1, 1]]), Bbox([[0, 0], [-1, -1]]))

>>>

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):

>>> Bbox.union([Bbox([[2, 2], [1, 1]]), Bbox([[0, 0], [-1, -1]])])
Bbox([[-1.0, -1.0], [2.0, 2.0]])

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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@brunobeltran
Copy link
Contributor Author

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.

@brunobeltran brunobeltran marked this pull request as draft April 13, 2020 14:18
@QuLogic
Copy link
Member

QuLogic commented Apr 14, 2020

>>> Bbox.intersection(Bbox([[2, 2], [1, 1]]), Bbox([[0, 0], [-1, -1]]))
Bbox([[-1.0, -1.0], [2.0, 2.0]])

Is this supposed to say Bbox.union? It doesn't jive with the text explanation.

Copy link
Member

@story645 story645 left a 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.

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 "
Copy link
Member

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?"

@anntzer
Copy link
Contributor

anntzer commented Apr 16, 2020

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.

@anntzer
Copy link
Contributor

anntzer commented Jun 11, 2022

Looking at this again, while I agree that requiring "oriented" bboxes may be best, perhaps an easier way out for now is to make Bbox([np.nan, np.nan], [np.nan, np.nan]) be the "null bbox" (such that intersection with it is null and union with it ignores it, by carefully using nanmin/nanmax as needed in the implementations).

@github-actions
Copy link

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.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jul 24, 2023
@anntzer
Copy link
Contributor

anntzer commented Jul 24, 2023

I think some form of this would definitely be good to have.

@anntzer anntzer added keep Items to be ignored by the “Stale” Github Action and removed status: inactive Marked by the “Stale” Github Action labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep Items to be ignored by the “Stale” Github Action status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants