Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Compute area of path #16859
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?
Compute area of path #16859
Changes from all commits
eb92405
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there some external resource we can link to that describes the formula?
While it's reasonable to have it documented here, I'd also anchor to some public description if possible.
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 added a link to a publicly accessible course notes on computer graphics. The terminology is slightly different, but the approach looks the same to me.
https://scholarsarchive.byu.edu/facpub/1/
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.
Would it be reasonable to calculate the area on a shifted path, which has it's "center" in the origin? If we are far away from the origin, we add and subtract a lot of long-and-small areas. I can imagine that this could significantly reduce numerical precision.
Check warning on line 712 in lib/matplotlib/path.py
lib/matplotlib/path.py#L711-L712
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.
Do you know if
iter_bezier
ensures thatprev_point
is correct forCLOSEPOLY
? It's not technically required for the point to be anything useful as it's ignored.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 logic is handled by
iter_bezier
, so if you receive aCLOSEPOLY
from that function, the "real" values for the start and end-points of the closing line are used, not whatever ignored value is passed by the user.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.
Test a simple non-curved polygon such as a triangle:
Side-remark: Not an expert, but I believe there are more efficient formulas for simple polygons without Bezier segments.
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 agree, but looking through this it is all vectorized and over the degree-1 shapes I'm guessing it isn't that expensive. I think this should also be a good way for someone to later come in and add a fast-path for those cases:
if CURVE == ...: simple formula
. Starting with this seems reasonable to me.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.
Digression, but this is apparently messing up mypy stubtest which I think is due to the float in here vs int in all the other paths. Changing this to
tuple(point)
instead satisfies mypy. Seems like typing gone wrong here.Do we really need to be running mypy over the test suite?
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 don't have a strong opinion on mypy on tests. I assume the added value would be that we check for reasonable typing of our interfaces by exercising calls to these interfaces.