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

Use geodesic conversion of meters, avoid empty polygon #60

Merged
merged 7 commits into from
Oct 29, 2024

Conversation

Sujanadh
Copy link
Contributor

@Sujanadh Sujanadh commented Oct 28, 2024

Description:

This PR uses geodesic conversion formula to convert meters to degrees (lat, lon) instead of hardcoded degree to convert meters to degree. Additionally, it fixes the issue of appending empty polygons to geojson output.

Updates:

Geodesic conversion includes the use of WGS:84 Ellipsoidal parameters such as :

- lat_rad: reference latitude in radian
- semi major axis : a = 6378137.0 m
- flattening factor : f = 1 / 298.257223563
- squared eccentricity (e2) : 2 f - (f ^ 2)
- radius of curvature in prime vertical (N) : a /sqrt(1 - e2 * sin(lat_rad)^2)
- radius of curvature in the meridian (M) : a * (1 - e2) / (1 - e2 * sin(lat_rad)^2)^(3 / 2)

image

Issue:

@Sujanadh Sujanadh requested a review from spwoodcock October 28, 2024 08:42
@Sujanadh Sujanadh self-assigned this Oct 28, 2024
@github-actions github-actions bot added the bug Something isn't working label Oct 28, 2024
Copy link
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

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

Wow, this is seriously impressive! Good first principals approach 🎉

@spwoodcock
Copy link
Member

It does break the tests for split_by_square though: https://github.com/hotosm/fmtm-splitter/actions/runs/11550316031/job/32145058693?pr=60

The split is generating 60 squares in the test instead of the previous 50 we had hardcoded to check against.
Any idea why we might get 10 more squares?

@Sujanadh
Copy link
Contributor Author

@spwoodcock , Does it necessarily need to produce same output from using multi polygon aoi and single polygon aoi (that are used in test cases)?

@Sujanadh
Copy link
Contributor Author

Previous response:
image

Recent changes:
image

It seems that the recent changes creates more perfect square than previous one so increasing the number of tasks.

@Sujanadh
Copy link
Contributor Author

Sujanadh commented Oct 29, 2024

For the mulipolygon aoi, the square splits result is not that good, previously also and I think we need to update that. For now we split individual polygon from multi polygon aoi and append it and the result is not that promising:
image

instead of doing individual polygon split, i think its better to create single aoi first and use it in splitting? what do you think?
We might want this to continue if the multi polygons are disjoint , but i think thats rare in such case to choose split by square?

@spwoodcock
Copy link
Member

Previous response:
image

Recent changes:
image

It seems that the recent changes creates more perfect square than previous one so increasing the number of tasks.

This one is great! Perhaps the squares are a bit smaller due to the more accurate representation of metres (the hardcoding previously must have been a bit inaccurate)

@spwoodcock
Copy link
Member

For the mulipolygon aoi, the square splits result is not that good, previously also and I think we need to update that. For now we split individual polygon from multi polygon aoi and append it and the result is not that promising:
image

instead of doing individual polygon split, i think its better to create single aoi first and use it in splitting? what do you think?
We might want this to continue if the multi polygons are disjoint , but i think thats rare in such case to choose split by square?

The idea was that in theory users could upload multiple distinct AOIs that aren't connected, within a featcol or multigeom. Say they want to map areas a b and c within a city.

But you are probably right, this scenario is less likely and we haven't encountered it yet!

Split by square needs to work reasonably well, but isn't the highest priority, so let's take note of that and leave it for now 👍

@spwoodcock
Copy link
Member

I think the tests can just be updated to match the new outputs, then this can be merged 😃

@Sujanadh
Copy link
Contributor Author

ok i will just update the test case then

@rsavoye
Copy link
Contributor

rsavoye commented Oct 29, 2024

Actually I am using multipolygons where the polygons aren't connected, but had to write my own splitting code since fmtm-splitter previously kept forcing me to make a data extract at the same time. Think field mapping amenities in multiple small towns... Sorry for the hacked meters/degrees conversion, at that time I figured for splitting by squares, accuracy wasn't important. After recent changes, I'm hoping to go back to using fmtm-splitter at some point.

@spwoodcock spwoodcock merged commit aa0db4c into main Oct 29, 2024
4 of 5 checks passed
@spwoodcock spwoodcock deleted the fix/square-split branch October 29, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants