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.

Sign up for GitHub

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

Jump to bottom

Buffer implied pointsets, and allow sub-pointsets to be uncontained b… #225

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Buffer implied pointsets, and allow sub-pointsets to be uncontained b… #225

wants to merge 2 commits into from

Conversation

mattwigway
Copy link
Contributor

This does two things:

  1. Buffers the implied WebMercatorGridPointset so that it expands beyond the street network extent (Buffer transport network when creating linked grid pointset #224). I used the max offstreet walk distance rather than the distance table size, because if points are further than that from the extent of the street network they are perforce unusable.
  2. Allows sub linked pointsets to not completely overlap the superpointset. Currently generating Browsochrones data near the edge of the graph fails because the sub point set created around the origin to report non-transit times have points outside the implied pointset. This way those points outside the graph pointset are simply left unlinked. The reason this is a problem now and not in the past is that we recently made a change to simply subset the implied linked pointset, rather than generating a new linked pointset, when computing the non-transit times. Thus, we won't see walk paths to areas outside the extent of the implied pointset anymore, but this seems like a small loss - not linking a new, small grid pointset was a performance win.

@mattwigway
Buffer implied pointsets, and allow sub-pointsets to be uncontained b…
1c9496b 
…y super pointsets

(so that the non-transit component of a browsochrones search will not cause exceptions when undertaken
near the edge of the graph, with part of the non-transit result grid outside the graph)
@mattwigway
Use max offstreet walk not distance table size to buffer implied poin…
062a1a1 
…tset.
@mattwigway mattwigway requested a review from abyrd January 30, 2017 22:31
abyrd
abyrd reviewed Feb 1, 2017
View reviewed changes
Copy link
Member

@abyrd abyrd left a comment

Choose a reason for hiding this comment

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

Looks good to me, added a couple of comments. The main thing I notice is that Github really mangles commit subject lines longer than about 55 characters, and while the commit messages can be expanded (breaking awkwardly in the middle of words) the title of the pull request has beed derived from one of the commits and cannot be expanded:

image

src/main/java/com/conveyal/r5/analyst/WebMercatorGridPointSet.java
double lonBuffer = SphericalDistanceLibrary
.metersToDegreesLongitude(LinkedPointSet.MAX_OFFSTREET_WALK_METERS, transportNetwork.streetLayer.envelope.centre().y);

int west = lonToPixel(transportNetwork.streetLayer.envelope.getMinX() - lonBuffer);
Copy link
Member

Choose a reason for hiding this comment

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

This could also be done with

        Envelope envelope = new Envelope(transportNetwork.streetLayer.envelope);
        envelope.expandBy(lonBuffer, latBuffer);

src/main/java/com/conveyal/r5/streets/LinkedPointSet.java
@@ -174,10 +176,22 @@ public LinkedPointSet(LinkedPointSet sourceLinkage, WebMercatorGridPointSet subG
distances0_mm = new int[nCells];
distances1_mm = new int[nCells];

// mark all points as unlinked, in case part of the subGrid lies outside the super grid. Points within the super
// grid will be overwritten below
Arrays.fill(edges, -1);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should be using a symbolic constant like UNLINKED for this, but I guess we have hard-coded -1s all over.

@abyrd
Copy link
Member

abyrd commented Jun 5, 2017

@mattwigway I'm going through these older pull requests, hoping to merge some of them. Is this one still applicable? Will there be any side effects to merging it (interference with other changes in the front end for example)?

@abyrd
Copy link
Member

abyrd commented Oct 13, 2020

Revisiting this in October 2020, after combining r5 with analysis-backend. These changes may still be useful but my previous comment still stands - it will take some time to determine whether this is applicable or will necessitate changes elsewhere in the system.

@abyrd abyrd closed this Apr 9, 2021
@abyrd abyrd deleted the branch dev April 9, 2021 04:50
@abyrd abyrd reopened this Apr 9, 2021
@abyrd abyrd changed the base branch from master to dev April 9, 2021 05:19
@abyrd
Copy link
Member

abyrd commented Apr 9, 2021

Accidentally closed when cleaning up branches. I would like to revisit these old pull requests and determine whether they're still relevant.

@ansoncfit
Copy link
Member

This may be superseded by #684 and #688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abyrd abyrd abyrd left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cleanup standby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mattwigway @abyrd @ansoncfit @landonreed