29 clip osm #34
29 clip osm #34
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #34 +/- ##
==========================================
- Coverage 83.91% 83.42% -0.49%
==========================================
Files 8 9 +1
Lines 491 525 +34
==========================================
+ Hits 412 438 +26
- Misses 79 87 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
|
||
| def filter_osm( | ||
| pbf_pth=here("tests/data/newport-2023-06-13.osm.pbf"), | ||
| out_pth="filtered.osm.pbf", |
There was a problem hiding this comment.
The default of out_pth is only the filename, so the output will be saved in root. Could this be changed to outputs/osm?
#80
|
|
||
| def filter_osm( | ||
| pbf_pth=here("tests/data/newport-2023-06-13.osm.pbf"), | ||
| out_pth="filtered.osm.pbf", |
There was a problem hiding this comment.
If using a path in the out_pth argument, using backslashes in the string is not recognised as a path and instead creates a file with that character or whatever escape character happens in the filename. Using double slash to prevent escape character just creates a file with the slash in the name.
Will add to #61 as it's related to helper function _check_parent_dir_exists.
| @@ -71,9 +75,9 @@ def _is_gtfs_pth(pth, param_nm, check_existing=True): | |||
| _, ext = os.path.splitext(pth) | |||
| if check_existing and not os.path.exists(pth): | |||
| raise FileExistsError(f"{pth} not found on file.") | |||
There was a problem hiding this comment.
This error message is unclear.
>>> filter_osm(pbf_pth = 'folder/subfolder/blah.pbf')
FileExistsError: folder/subfolder/blah.pbf not found on file.
Something like <file> not found would read better.
| cmd = [ | ||
| "osmosis", | ||
| "--read-pbf", | ||
| pbf_pth.as_posix(), |
There was a problem hiding this comment.
PosixPath() will likely not work in Windows machines. According to the pathlib documentation, the method .as_posix returns a string representation of the path with forward slashes. Perhaps worth exploring a more OS agnostic method for this.
#81
| except FileNotFoundError as e2: | ||
| if install_osmosis: | ||
| print(f"osmosis command was not recognised: {e2}. Trying install.") | ||
| subprocess.run(["brew", "install", "osmosis"]) |
There was a problem hiding this comment.
subprocess.run(["brew", "install", "osmosis"]) will only run in macOS systems. We could add a more system agnostic way of doing this? E.g. check OS and use an if statement to run different commands depending on OS.
#82
| @@ -40,13 +40,21 @@ jobs: | |||
| - name: Check Java Install | |||
| run: | | |||
| java --version | |||
| - name: Install Osmosis | |||
There was a problem hiding this comment.
Instructions to install osmosis need to be added to CONTRIBUTING.md.
#79
| filter_osm(bbox=[0.0, 0.1, 0.1, 0.0]) | ||
| with pytest.raises( | ||
| TypeError, | ||
| match="ox` must contain <class 'float'> only. Found <class 'int'>", |
There was a problem hiding this comment.
This will work, but truncating the error message to match to fit the line does not look good.
| _is_expected_filetype(pth=in_pth, param_nm="in_pth") | ||
| _is_expected_filetype( | ||
| pth=out_pth, param_nm="out_pth", check_existing=False | ||
| ) | ||
| _check_list(ls=bbox_list, param_nm="bbox_list", exp_type=float) | ||
| for param in [units, crs]: | ||
| if not isinstance(param, str): |
There was a problem hiding this comment.
If this function is going to be used to crop GTFS based on a provided box, we should rename the local variable newport_feed to something generic.
Description
Fixes #29
filter_osm()function.filter_osm()Motivation and Context
Filter Open Street Map pbf files to a bounding box.
Type of change
How Has This Been Tested?
Test configuration details:
Advice for reviewer
Pytest modules renamed.
transport_performance.gtfs.utilsandtransport_performance.osm.utilswould cause problems. Therefore gone with:transport_performance.gtfs.gtfs_utils, so on.Checklist:
pkg_resourcesdeprecation warnings have started again. Please see pkg_resources deprecation warning #19 . Note that this is a local issue on my machine and is not evident in the CI runners.Additional comments