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 pixel centers as points in web Mercator grids #894

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Conversation

abyrd
Copy link
Member

@abyrd abyrd commented Oct 17, 2023

Travel time and accessibility results will be slightly different when computed with workers after this change. Pre-built linkages in saved networks will not be affected - the entire network must be rebuilt to ensure they are consistent, so this requires a network version bump. However we are also updating to Kryo 5 serialization format, so if both changes are introduced at once in a release we need only bump the network version number once.

@abyrd abyrd changed the base branch from dev to wgs-mercator-stability October 17, 2023 11:42
@abyrd
Copy link
Member Author

abyrd commented Oct 17, 2023

Simpson desert tests are failing, which is not surprising as travel times are slightly different with this linking strategy. PR #896 changes those tests so they no longer depend on the position of the destination grid. That PR will need to be merged before this one's tests will pass.

All other uses of pixel edge functions now seem to be only for finding
bounds of pixels, the entire grid, or extents. Point positions should
now be uniformly in the center of Mercator pixels.
@abyrd abyrd force-pushed the mercator-pixel-center branch from 3b769bb to f23b3fc Compare October 19, 2023 17:17
@abyrd abyrd changed the base branch from wgs-mercator-stability to merge-pixel-functions October 19, 2023 17:18
@abyrd abyrd added the nv-update Implies changes to the serialized network (either format or logical content changes) label Oct 19, 2023
Base automatically changed from merge-pixel-functions to dev November 2, 2023 09:11
@abyrd abyrd marked this pull request as ready for review November 2, 2023 13:59
@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

Tests are now passing with the Simpson Desert test changes merged in. I think this is ready for final review.

@abyrd abyrd enabled auto-merge November 2, 2023 14:37
trevorgerhardt
trevorgerhardt previously approved these changes Nov 2, 2023
@abyrd
Copy link
Member Author

abyrd commented Nov 2, 2023

@trevorgerhardt merge commit to combine the use of extents with pixelToCenterX invalidated your review.

@abyrd abyrd merged commit 0cc9bdd into dev Nov 2, 2023
3 checks passed
@abyrd abyrd deleted the mercator-pixel-center branch November 2, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nv-update Implies changes to the serialized network (either format or logical content changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants