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

Add great-circle meridian crossing option #159

Merged
merged 10 commits into from
Dec 18, 2024

Conversation

teobouvard
Copy link
Contributor

@teobouvard teobouvard commented Dec 11, 2024

Related issues and pull requests

Description

This PR introduces support for splitting polygons crossing the meridian using spherical geometry rather than 2D, which provides nicer splits for large objects meridian diagonally.

For example, the polygon referenced in #153, which was originally fixed like this:

{"type": "MultiPolygon", "coordinates": [[[[180.0, -65.726774], [179.45064648, -65.47124346], [178.30851744, -65.99077528], [179.74817611000003, -66.51221218], [180.0, -66.2469064], [180.0, -65.726774]]], [[[-180.0, -66.2469064], [-179.1162027, -65.98206677], [-180.0, -65.726774], [-180.0, -66.2469064]]]]}
Loading

would now look like that:

{"type": "MultiPolygon", "coordinates": [[[[180.0, -65.6710376], [179.45064648, -65.47124346], [178.30851744, -65.99077528], [179.74817611000003, -66.51221218], [180.0, -66.397275], [180.0, -65.6710376]]], [[[-180.0, -66.397275], [-179.1162027, -65.98206677], [-180.0, -65.6710376], [-180.0, -66.397275]]]]}
Loading

Checklist

  • Add tests
  • Add docs
  • Update CHANGELOG

This commit adds an option to compute the intersection of a segment and
the meridian using spherical geometry, which provides nicer splits for
large crossing the meridian diagonally.

Related to gadomski#153
@teobouvard
Copy link
Contributor Author

@gadomski I have local changes which propagate a CLI option --great-circle through all functions down to crossing_latitude(). Is this how you want to proceed, or do you have something else in mind ?

@gadomski
Copy link
Owner

I have local changes which propagate a CLI option --great-circle through all functions down to crossing_latitude(). Is this how you want to proceed, or do you have something else in mind ?

I think it should be the default behavior, it looks much nicer. So the CLI option would be better if is disabled great circle behavior, IMO.

This propagates the decision of whether to use planar or spherical
geometry to compute meridian crossing latitudes.
The default is set to False to keep this commit self-contained, because
changing the default has a significant impact on the test results.
The change of default will be done in a later commit, which will update
the test suite at the same time.

See gadomski#159 (comment)
This provides a way to opt out of the default spherical geometry used
for computing meridian crossings.
This prepares the addition of test outputs expected from spherical
geometry.
This is required to introduce an optional argument with a default value
(subdirectory), which cannot be expressed with Callable type hinting
only.

This has the nice benefit of not removing the need for typing
extensions, since Protocols are available since Python 3.8, which is
lower than the oldest non-EOL Python version.

See https://docs.python.org/3/library/typing.html#annotating-callable-objects
The output of ogr2ogr is consistent with the behaviour of great-circle
meridian crossing computation introduced by this PR.
The differences have been checked visually and are convincing, except
the force-north-pole test for which a double-check would be needed.
Some tests were kept unparametrized over great_circle to make sure that
the default behaviour is also tested.

See gadomski#159 (comment)

BREAKING CHANGE: Meridian crossings are now computed using spherical
geometry by default.
@teobouvard
Copy link
Contributor Author

All code-related changes should be in. How do we go about updating the docs ? The drawings in the-algorithm.md are in 2D, and I don't think I would be able to match their style. We could probably do a nice visualization using something like manim, but it will take some time. I'm not sure it should be part of the PR.

@teobouvard teobouvard marked this pull request as ready for review December 12, 2024 16:13
@teobouvard teobouvard requested a review from gadomski as a code owner December 12, 2024 16:13
@gadomski
Copy link
Owner

I'm not sure it should be part of the PR.

Nope, that's ok! No need to do the docs, I'll take that on if needed. Really appreciate it!

Copy link
Owner

@gadomski gadomski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really excellent 🥳! Thanks for the sidecar typing fixes, etc.

@gadomski gadomski merged commit a054124 into gadomski:main Dec 18, 2024
9 checks passed
@gadomski
Copy link
Owner

@teobouvard teobouvard deleted the feat-great-circle branch December 18, 2024 18:06
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.

Use great circles to calculate crossing latitude
2 participants