-
Notifications
You must be signed in to change notification settings - Fork 535
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
Refactor window intersection and union #2164
Conversation
@sgillies Could I get a review on this? |
@sgillies I didn't mean to interrupt your vacation. This is independent of #2162. Two main things happen in this PR:
|
return Window.from_slices( | ||
(stacked[0, 0].max(), stacked[0, 1].min()), | ||
(stacked[1, 0].max(), stacked[1, 1]. min())) | ||
return functools.reduce(_intersection, windows) |
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.
👍
if coeffs[2] > 0 and coeffs[3] > 0: | ||
return Window(*coeffs) | ||
else: | ||
raise WindowError(f"Intersection is empty {w1} {w2}") |
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.
@groutr combine these as the first is only called from the second?
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 you're referring to why I split the functions up the way I did.
The reasoning is as follows:
_compute_intersection
: Compute the intersection directly. A low-level function that is useful in the case where it is not desired to create a Window object. This is useful in the case where the calling method is creating its own Window object or manipulating the values. Reviewing the function, it should probably be modified to also work on the tuple form of a Window.
_intersection
: Compute an intersection and check if the intersection is empty. It is a higher-level function that is primarily designed to receive as input Windows and return a Window. This is what makes the reduce
call possible.
I can combine them if you feel the current split is unnecessary.
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.
@groutr we deprecated the old tuple form at 1.0: https://github.com/mapbox/rasterio/blob/master/CHANGES.txt#L1082. No need to support it.
intersection(*windows) | ||
return True | ||
except WindowError: | ||
return False |
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.
Yes!
@groutr what would you think about rebasing this on the master branch, for 1.3? That would protect us a little more from regressions. |
Pass Window objects to intersection.
@sgillies I rebased onto master. I'm fine with holding these changes for 1.3 as they don't directly fix any known bug. |
@sgillies Is this PR still destined for 1.3.0? |
@groutr yes, thanks for the query! |
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.
Let's do this! Thanks @groutr.
Window intersections and unions used suboptimal implementations.