-
Notifications
You must be signed in to change notification settings - Fork 19
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
Eliminate redundant transformations in area calculation #437
Conversation
When calculating an area with a transformation, we only really have to transform each point exactly once. However, the previous implementation was naively calculating each transformation twice. For simple transformations, this may not matter much. However, the transformation is user-supplied, so could be arbitrarily expensive to compute. The bounds of the for loop in the shoelace formula implementation have been reduced by 1 from from `[0, n)` to `[0, n-1)`. This is because the shoelace formula applies to rings, and the start and end points of rings are always the same. So the final iteration in the previous implementation was redundant.
})) | ||
|
||
// Each of the 4 points making up the polygon get transformed once each. | ||
expectIntEq(t, count, 4) |
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've probably misunderstood something, but in my mind, would have expected count
to be 3 since the POLYGON is a triangle (3 vertices) made up of 4 points and we're looping 4-1=3 times?
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.
You're on the right track, but I think you may have missed that nthPt
is called once before the loop starts. So yes, we do loop n-1
times, but we do 1
transformation outside the loop as well.
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.
Ah yes, the pt1 := nthPt(0)
. Thanks for clarifying!
LGTM, and I'm sure I'm wrong (re: my comment), but just wanted to clarify why my understanding is wrong. :) |
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.
LGTM
Thanks for the review! |
Description
Check List
Have you:
Added unit tests? Yes, a new unit test to check the number of invocations. Existing unit tests ensure that the result of the area calculation is still correct.
Add cmprefimpl tests? (if appropriate?) N/A
Updated release notes? (if appropriate?) Yes.
Related Issue
Benchmark Results
N/A