Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ee0a30c
to3394535
CompareThere 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.
Some comments that might be useful to a reviewer.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
296baeb
to37d533f
Comparecb14bb2
toaebcbc7
Compare4c80f38
to08e61bc
Compare1c30420
to4a41a2f
Comparelib/matplotlib/tests/test_bezier.py Outdated
dr = dB(t).T | ||
n = np.array([dr[1], -dr[0]]) | ||
return (B(t).T @ n) / 2 | ||
return scipy.integrate.quad(integrand, 0, 1)[0] |
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.
Testing our area formula (bezier.BezierSegment.arc_area
) is most naturally done by comparing to the naive integral implementation. However, this required me to addscipy
to the testing dependencies.
I already test this formula (indirectly) intest_path.test_signed_area_curve
andtest_path.test_signed_area_unit_rectangle
, so if we prefer to not add any dependency on scipy at all, I can simply remove this test.
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.
It was agreed on a call back in June that it's okay to add this Scipy dependency as long as it's just an optional testing dependency.
4cf12f3
to0037abd
Comparebrunobeltran commentedSep 1, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@QuLogic when you get tired of working with the Mac stuff, this is rebased and the math should now render fully (making it marginally easier to review), and I incorporated all of your recommendations. |
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. |
This was a lot of good work that seems useful to get in. I just rebased this PR and replaced the manual combinatorial function with |
7054e98
to40275b8
CompareThere 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.
Overall, I'm +/-0 on adding this. Do we really need the area? It was pitched for equal-area markers. But I'm not sure on the cost/benefit. As mentioned above there are some potential rabbit holes if we provide this as a public interface. If the primary use would be equal-area markers, I suggest to make this private for now.
Notes | ||
----- | ||
An analytical formula is possible for arbitrary bezier curves. |
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/
prev_point = None | ||
prev_code = None | ||
start_point = None | ||
for B, code in self.iter_bezier(): |
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.
# a point | ||
Path([[1, 2]], [Path.MOVETO]), | ||
_ExampleCurve(Path([[1, 2]], [Path.MOVETO]), extents=(1., 2., 1., 2.), | ||
area=0.), | ||
] |
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:
] | |
_ExampleCurve(Path([[1,1], [2,1], [1.5,2]]),extents=(1,1,2,2),area=1/3), | |
] |
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'tthat 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 totuple(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.
cdf05fe
todf8ea60
Compare@timhoffm, I share some of your concern, but overall feels like this is an improvement worth adding. If someone wants more complex geometry handling they should likely move over to Shapely and GEOS. But this has the work put into it and I could see people not wanting to have to convert a Matplotlib Path over to Shapely and go back and forth if they can avoid it. For the equal area markers: I think that is one of the most requested scatter marker "features", so I think people definitely would want it. |
70f327d
toa489cbe
Comparetimhoffm commentedOct 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We have to be careful what we offer. If we provide a public functionality it should "just work" and we need to document it's scope and limitations. This is more work than just writing it for internal usage. For example, the current Similarly, marker paths are typically defined in the vicinity of the origin, sowe would not have to care for the possible numeric issue with far-away paths. Also, the precision requirements for reasonable equal-area markers are low. A few percent deviation are tolerable for our use case, but it may not in user applications. Generally, geometric calculations are not trivial. I'm quite afraid we provide half-baked solutions, that do not satisfy user needs and will result in follow-up requests to improve on that functionality, which is not our core business. |
The ROSES 2024 grant haspath transformations as an area of focus. @greglucas if this issue is part of the work that will help w/ that, could you create a roadmap issue of what should be implemented in Matplotlib to facilitate that work? |
No this isn't related to that. The PR after this (path_length) would be more related, but not directly as that work is all done within Cartopy. I don't personally have a direct benefit from this at this point in time, so I will just leave the rebased version here in case someone else wants to advocate for it again in the future or use it as a reference, I don't feel strongly enough to advocate for this right now. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Add method to
.Path
that can compute the exact (signed) area of the path "fast" (i.e. without any integrations or discretization required) using the simple analytical formula for general Bezier curves.Required for implementation of "equal area" markers (my current proposal for dealing with#15703).
Roadmap:
#16812 (*) <-#16832 (*) <- (This PR) (*) <-#16891 (MarkerStyle improvements!)
"<-" means "depends on", and "(*)" marks PRs whose implementations are complete and fully ready for review.
PR Checklist