PBF To Atlas Sub Commmand #223
Conversation
There was a problem hiding this comment.
Thanks @Bentleysb really clean code! Just a few changes requested.
|
|
||
| // Load Parameters | ||
| private static final Switch<Boolean> LOAD_RELATIONS_PARAMETER = new Switch<>("load-relations", | ||
| "Whether to load Relations (boolean)", Boolean::new, Optionality.OPTIONAL); |
There was a problem hiding this comment.
How about Boolean::parseBoolean and add a default value of "true" at the end? That would make the switches well defined for the end user!
|
|
||
| // Filter parameters | ||
| private static final Switch<File> EDGE_FILTER_PARAMETER = new Switch<>("edge-filter", | ||
| "Path to a json filter for determining Edges", File::new, Optionality.OPTIONAL); |
There was a problem hiding this comment.
I would mention that it has to be a local File here in the description.
| @Override | ||
| public int execute(final CommandMap map) | ||
| { | ||
| new OsmPbfLoader((File) map.get(INPUT_PARAMETER), this.getAtlasLoadingOption(map)) |
There was a problem hiding this comment.
Here, I think we should use the latest ingest, that goes through raw Atlas. I think this is the way (@MikeGost can correct me):
File pbf, output;
AtlasLoadingOption option;
Atlas atlas = new RawAtlasGenerator(pbf, option).build();
if (countrySlicing)
{
atlas = new RawAtlasCountrySlicer(countryList, countryBoundaries).slice(atlas);
}
atlas = new WaySectionProcessor(atlas, option).run();
atlas.save(output);@MikeGost Maybe we should have an easy one-liner available that does all that somewhere, when the pbf->atlas loading use case is simple like that.
There was a problem hiding this comment.
Agreed, this should flow through the raw atlas flow instead of the legacy atlas creation mechanism. This is also strikingly similar to the work that @lucaspcram did here: osmlab/atlas-generator#53. We may want to consolidate the tools into a single one.
There was a problem hiding this comment.
See RawAtlasIntegrationTest for some examples of the flow.
There was a problem hiding this comment.
@matthieun - I have a backlog item to refactor AtlasLoadingOption that would automatically allow you to build complete atlases, without having to perform each step (create, slice, section) yourself. Definitely valuable functionality that can be reused here.
There was a problem hiding this comment.
It looks easy enough to change this over to the raw atlas flow. The bigger questions seems to be if we want to consolidate commands. I did not realize the atlas generator command existed, and I agree it has very similar functionality. The one big difference I see is that the atlas generator command is built around the sharded pbf/atlas workflow, whereas this command is made to be a more general flow.
| @@ -0,0 +1 @@ | |||
| UTF-8 | |||
There was a problem hiding this comment.
Can we remove all the shapefiles? I think the text boundary below should be enough..
There was a problem hiding this comment.
Nice command! These changes are similar to the RawAtlasCreator and AtlasCreator commands in atlas-generator. But you have added some additional features the aforementioned commands do not have (the filtering options). I think it's worth discussing integrating these tools in the future.
Description:
This adds a simple command to convert an OSM PBF to an Atlas file (as per #88). It requires only the path to a PBF file, and a path for an output file. It has a number of optional parameters that can be passed to customize the Atlas generation.
Potential Impact:
No impacts for downstream libraries.
Unit Test Approach:
Unit test
testDefaultConversionchecks that that the command functions as intended when no optional parameters are passed.Unit test
testFiltersTextMapCountryCodesConversionchecks that all the filter parameters, country code parameter, and the country boundary map parameter with a text file work as expected.Unit test
testNoSectioningNoSlicingNoRelationsShapeMapConversionchecks that the way section, country slicing, load relation, and country boundary map (with a shape file) parameters work as expected.Unit test
testNoWaysConversionchecks that the load ways parameter works as expected.Test Results:
Local tests were performed on multiple PBFs, using many combinations of default and optional parameters. All worked as expected, with the exception of using a large shape file as a country boundary map. This scenario caused the command to take a long time to load. It is recommended to use text country boundary maps when possible.
In doubt: Contributing Guidelines