186 filter to date method in gtfsinstance #190
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #190 +/- ##
==========================================
+ Coverage 97.77% 97.81% +0.03%
==========================================
Files 14 14
Lines 1171 1188 +17
==========================================
+ Hits 1145 1162 +17
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.
Hi @r-leyshon . Great work on this. Everything is working as expected.
I have a few small comments however they aren't huge things to clear up.
On the r5py stuff, from eyeballing it, it looks good to me. We might however want to get someone else to quickly run the test on their machine to ensure it works.
This comment was marked as resolved.
This comment was marked as resolved.
|
@CBROWN-ONS I have implemented all your suggestions except the final comment, which unless I have misunderstood is already the case. Many thanks for the good catches once again. Did you feel you needed a more in depth review of the tests or does a passing CI suite do enough to demonstrate the behaviour is as expected? |
Hi Rich. After reviewing your comments on the matter, I agree that we are fine not to accept a parameter for date format while filtering the GTFS. Great work on the other changes. Regarding the r5py stuff, I'm happy to merge as the CI demonstrates that these tests pass and are operational. Let me know if you agree with this, and I will merge this into dev. |
Good to go from my end, thanks again. |
* feat: Optionally filter gtfs to date list * refactor: Toml has no nonetype, so empty list used to specify no date filter * test: Invalid datestring causes raise * test: Raises if filter_dates values do not exist within gtfs calendar * refactor: All in_pth values use gtfs fixture path constant * test: Assert filtered net can build r5py network * fix: Runinteg invocations in workflows needed fixing * fix: Runner doesn't install osmosis so deselect all tests with osmosis deps * feat: Changes on bristol run * refactor: Defense checks & type hinting * docs: Add docstring to internal * chore: Move _validate_datestring to defence.py * docs: Update docstring --------- Co-authored-by: Richard Leyshon <[email protected]> e6a700b
This reverts commit e6a700b.
Description
bbox_filter_gtfs optionally filters to a list of dates present within the gtfs calendar.
Fixes #186
Motivation and Context
Reducing GTFS archives to valid features (stops, shapes etc) on a given date appears to reduce the likelihood of encountering r5py errors when building transport network. It is currently thought that these errors are encountered due to specific fast travel issues, although further investigation and reproduction of the encountered error is ongoing.
Type of change
bbox_filter_gtfs will continue to work as before, although specifying a populated list will run the
gtfs_kit.feed.restrict_to_date()logic.How Has This Been Tested?
Defensive checks and test suite included. Also a test marked with runinteg to ensure the r5py trasport network can be built from the filtered GTFS file.
Test configuration details:
Advice for reviewer
Upon submission I noticed the test runners failing. After reviewing the fails, it became apparent that the current pytest invocation was incorrect and breaking. The changes to the CI workflows fix that and remove duplicated tests in all-os.tests.yml, where tests were being run twice:
Checklist:
Additional comments