Skip to content
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

Ndr/misc performance #123

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Ndr/misc performance #123

merged 5 commits into from
Feb 16, 2024

Conversation

nreinicke
Copy link
Collaborator

Tacks on a couple more performance upgrades:

  1. Use f32 instead of f64 to hold onto our coordinates. This gives us about 5 decimal places when using lat/lon as our internal coordiantes which is around a 1 meter resolution. Eventually we could adopt an internal projection and store the coordinates as u32 in centimeters (the earth circumference is about 4e9 centimeters and the maximum value for a u32 is about 4.3e9).
  2. Use u8 for road class representation. This will improve memory performance and runtime performance since we hash the road class at every search iteration to make sure it's valid. I also added an extension point for a user provided mapping from string road classes into u8 road classes in case we want to provide them as a string in the input file (like for OSM).
  3. Implement an iterator for graph.out_edges / graph.in_edges. This improves the runtime since we don't have to allocate a vector of edge ids at each search iteration, we just iterate over them lazily.

Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some sensible improvements.

per our discussion, i would like to be sure we avoid issues with floating point accuracy, as talked about here with using f32. in particular, if all geometries are represented in f32, does it mess with our route linestrings?

and, just a little nudge, why not finish whatcha started with the road class mapping? i think it's close?

@@ -18,7 +24,7 @@ impl FrontierModelService for RoadClassFrontierService {
) -> Result<Arc<dyn FrontierModel>, FrontierModelError> {
let service: Arc<RoadClassFrontierService> = Arc::new(self.clone());
let road_classes = query
.get_config_serde_optional::<HashSet<String>>(&"road_classes", &"")
.get_config_serde_optional::<HashSet<u8>>(&"road_classes", &"")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to implement the mapping action, when road_class_mapping is not None, do we replace this logic with the mapping logic, and then set "road classes" from the result of the mapping? seems like just a few more lines to add to the service here.

@nreinicke
Copy link
Collaborator Author

I did a quick check with using f32 coordinates and I'm not seeing any noticeable differences in our linestrings. Here's a zoomed in example:

f64 geometry

image

f32 geometry

image

Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, i'm convinced we haven't nuked the world. great catches here!

@nreinicke
Copy link
Collaborator Author

@robfitzgerald - flagging this for another quick look. I added the ability include a mapping from string road class to integer road class so users can spec a string road class at query time. This might conflict with some recent updates you're making to the input plugins.

@nreinicke nreinicke merged commit e5898f9 into main Feb 16, 2024
5 checks passed
@nreinicke nreinicke deleted the ndr/misc-performance branch February 16, 2024 21:26
@robfitzgerald
Copy link
Collaborator

robfitzgerald commented Feb 16, 2024

hey, rolling in the road class changes breaks the configurations on the tomtom repo:

[2024-02-16T22:02:02Z ERROR routee_compass] Error: "error with parsing inputs: Unable to process EdgeRtree Input Plugin due to: error decoding input: \n                                    Could not parse incoming query valid road_classes as an array of integers \n                                    and this FrontierModel does not specify a mapping of string to integer. \n                                    Either pass a valid array of integers or reload the application with a \n                                    mapping from string road class to integer road class.\n        

those configs all inject road classes as strings, like this:

    { type = "inject", key = "road_classes", value = '["1","2","3","4","5","6"]', format = "json" },

but it works if you un-quote those numbers, so just a heads up until once of us updates the configs.

@nreinicke
Copy link
Collaborator Author

Oops, yeah good catch, let me update those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants