-
Notifications
You must be signed in to change notification settings - Fork 20
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
facility link snapping #276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean, just a few suggested minor changes.
thanks @brynpickering - I have made some updates in response to your comments. Before merging, @gac55 let me know if it works for your use case. |
Thanks @Theodore-Chatziioannou , picking this up now |
I've tested the new When I tried it on TE pop, I noticed it perform quite slow when scaling up to larger populations. It took about 30 minutes of runtime for 0.01% of the population for TE pop. I feel the primary slowdown seems to come from the way distances are calculated for each activity to all network links. Maybe some vectorized operations or something similar would help to potential speed-ups to process? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the snap_facilities_to_network performed slowly when it needed to work with big population inputs.
Now it utilizes cKDTree for efficient nearest neighbour search and batch processing of activity locations. This update improves performance in snapping facilities to the network geometry now. @Theodore-Chatziioannou
for act in person.activities: | ||
assert act.location.link is None | ||
|
||
snap_facilities_to_network(population=population_heh, network=network) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is a def
missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it just calls the functionsnap_facilities_to_network
here.
thanks @syhwawa
|
src/pam/operations/snap.py
Outdated
if not hasattr(point, 'x') or not hasattr(point, 'y'): | ||
point = point.centroid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? Loc should always be a point with x and y coordinates (or None
).
See here: https://github.com/arup-group/pam/blob/3f817a662317d44fca2126295f07d89317db707c/src/pam/location.py#L7-L18
.
If that is not the case in the input data, then the inputs should be corrected rather than fixing them silently here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed this.
Thanks for the comments @Theodore-Chatziioannou The function has been updated to remove the separate loops for collecting and applying data. Now, the updated locations are queried and assigned directly within the first loop. And the lint error has been fixed. |
Adds CLI method for snapping activity facilities to a network geometry.
It can be invoked as:
pam snap-facilities <path_population_in> <path_population_out> <path_network_geometry> -f <link_id_field>
For each activity, the method will identify the closest link in the shapefile, and add it as a
link=..
attribute to the activity element. Finally, the updated plans file will be saved to a new xml file.Checklist
Any checks which are not relevant to the PR can be pre-checked by the PR creator.
All others should be checked by the reviewer(s).
You can add extra checklist items here if required by the PR.