Skip to content
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

[turf-ellipse] fixes the wrong shape when drawing "big ellipses" near the poles #2739

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hadbn
Copy link

@hadbn hadbn commented Oct 25, 2024

-Fixes the wrong behavior of turf-ellipse when drawing "big ellipses" near the poles
-Fixes the wrong behavior of turf-ellipse when rotating an ellipse. The rotation is now fully taken account during the calculation of the geometry.
-Adds two test to directly check both previous points
-Fixes one of the existing test turf-ellipse which was not launched when launching pnpm test
-Changes the test turf-ellipse so that it fits the fixes made in the calculation of the geometry

Resolves #2736

-Fixes the wrong behavior of turf-ellipse when drawing "big ellipses" near the poles
-Fixes the wrong behavior of turf-ellipse when rotating an ellipse. The rotation is now fully taken account during the calculation of the geometry.
-Adds two test to directly check both previous points
-Fixes one of the existing test `turf-ellipse` which was not launched when launching `pnpm test`
-Changes the test `turf-ellipse` so that it fits the fixes made in the calculation of the geometry
@smallsaucepan
Copy link
Member

Thanks @hadbn. What a fantastic contribution.

Doing some comparisons between the old and new test fixtures, have the "*-degrees" tests been broken all along? This current test output suggests the ellipses have never been the right size longitudinally, let alone the correct shape?

Screenshot 2024-10-26 at 11 25 47 AM

Also, and I need to apologise for not mentioning it sooner, there is another ellipse issue open - #1767 It has a solution suggested, and I'm wondering if that could be incorporated into your PR without much extra effort?

Screenshot 2024-10-26 at 11 44 22 AM

It would mean (I think) adding an optional accuracy config option.

Again, sorry for not mentioning this sooner. Didn't expect you to surge ahead with a PR right away! If that's not possible for you let me know and I will have a look myself.

@hadbn hadbn changed the title [turf-ellipse] fixes the wrong when drawing "big ellipses" near the poles [turf-ellipse] fixes the wrong shape when drawing "big ellipses" near the poles Oct 26, 2024
@hadbn
Copy link
Author

hadbn commented Oct 26, 2024

To be honest, I didn't spend much time studying the old tests so I couldn't say what was functional and what wasn't. However, I did notice a lot of errors like the ones you point out, which motivated me to change how the tests are run. It might also be worth thinking about more "robust" tests. The “in”/“out” comparison tests only allow you to check that the geometry doesn't “change” according to the implementation, but they don't verify the accuracy of the geometry. It was with this in mind that I introduced the two tests of comparison with a circle and invariance by rotation. If you have any other idea of comparison feel free to tell me and I will add them.

I actually saw the other issue you are mentionning. As it's my first PR I wasn't sure if I could resolve two issues in a single PR, so I did not.
I will have a look at it and integrate the suggested modifications in my branch. I'll let you know.

@smallsaucepan
Copy link
Member

In this case closing two issues in the same module will be fine. And happy to stick with the tests the way you're doing them.

@hadbn
Copy link
Author

hadbn commented Oct 30, 2024

@smallsaucepan here are the modifications.
All ellipses are now calculated so that points are equally distributed along the circumference.
I based my work on the issue you mentionned earlier. However I had to do some modifications because the proposed implementation led to problems when considering big ellipses. Quickly, this was due to the fact that when considering "big" ellipses the distance between two points is quite different from the arc length between those points. This led to a difference between Ramanujan's circumference and the one calculated using the discretization.

Moreover, this method gives prettier ellipses but I have to admit that it's quite expensive.

Are you happy with that ?

@smallsaucepan
Copy link
Member

Thanks @hadbn. Discussing with the other maintainers about the performance impact.

…ence is now optional. The default value for accuracy (0) leads to the same result as previously. Using custom values for accuracy will distribute points along the circumference (which is more precise for "thin" ellipses, but more expensive).
@hadbn
Copy link
Author

hadbn commented Nov 4, 2024

@smallsaucepan as I was concerned about the cost of the new method, I updated the function. The parameter "accuracy" is now optionnal. When not used (or set to 0) the ellipse will be calculated using the historical method. When the parameter is used, the new method will be used.
It could also be relevant to use a threshold on the thickness of the ellipse, as the new method is usefull for thin ellipses but useless when considering "round" ellipses. However according to me the users of the function should decide the method they use by themselves, so I chose not to use the threshold in the function.

I am -of course- opened to every comment you (or other maintainers) could make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the result of turf/ellipse is not exact at high lattitudes
2 participants