173 analyse network #200
173 analyse network #200
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #200 +/- ##
==========================================
+ Coverage 98.18% 98.30% +0.11%
==========================================
Files 18 19 +1
Lines 1433 1530 +97
==========================================
+ Hits 1407 1504 +97
Misses 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I've completed a review of the source code updates. The main functionalities all LGTM. I've tested them with a few urban centres and:
- we're getting consistent results to those we got previously, and
- transport performance metrics are identical whether calculated using the batched mode or not.
Just sharing initial comments at this stage, as it would be useful to pick out which of these (if any at all) need to be implemented right away, tagged as tech-debt, or treat as 'won't fix'. Let me know your thoughts on this.
In the meantime I'll continue to review the unit tests and add comments
There was a problem hiding this comment.
I've reviewed this PR and all LTGM - thanks very much for putting this together, it's a really great job!
As well as reviewing the new features and tests in detail, i've also run back-to-back comparisons with previous results (when this module wasn't available). This gave identical outputs for multiple urban centres. This was also identical in terms of performance for the non-batched approach. I also compared transport performance outputs for batched and non-batched methods - again identical results for multiple urban centres.
Only 2 pieces of tech-debt remain (they do not impact the core functioning of analyse_network and will be dealt with as smaller tickets):
- Possible to overwrite parquet outputs when running
od_matrix()with different configurations - Overwriting existing parquet files when rerunningod_matrix()with different configurations. #222 - When batching, if there are no destination centroids within distance of the origin then it writes empty parquet files - Handle empty parquet files when batching and no destinations are within distance of origins #225
Also one enhancement off the back of this PR:
- OD matrix index naming when writing to parquet - Writing to parquet with a
__null_dask_index__#221
Happy to merge to dev. Thanks very much again for your help with this.
Description
Module analyse_network.
Closes #173
Fixes #211
Motivation and Context
This module is a wrapper for r5py to calculate an origin-destination matrix to feed into the matrix module. It takes the outputs from the urban_centres and rasterpop modules.
Module allows selection of a different number of origins to calculate O-D matrices in batches. It saves the outputs as parquet files, one per origin batch. However, it will split any parquet file exceeding a certain size.
Type of change
How Has This Been Tested?
Module run locally, outputs checked manually.
Unit tests run locally.
Test configuration details:
Advice for reviewer
Run times:
Checklist:
Additional comments
I've chosen to not explicitly ask for keyword arguments from r5py and leave them as
**kwargs. This may not be a good choice, but I wasn't sure if arbitrarily including some arguments and not others would be messy. Happy to change it.Regarding the defence against dates not being in the GTFS (which also triggers when no valid GTFS is passed and
TransportMode.TRANSITis included), it's not straightforward to access any attribute withinTransportNetworkorTravelTimeMatrixComputerthat exposes GTFS information. The current solution catches the specific runtime warning and raises anIndexErrorwhen present.