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/cost model performance #111

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Ndr/cost model performance #111

merged 4 commits into from
Feb 6, 2024

Conversation

nreinicke
Copy link
Collaborator

Applies some minor modifications to the cost model to use iterators instead of allocating a vector for the cost model aggregation.

Also flattens the state variable coefficients, vehicle rates and network rates into a fixed size vector for index based lookup.

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.

dig it, nice find. wondering, did this change make a dent in your profiles?

CostModel {
) -> Result<CostModel, CostError> {
// map the state variable coefficiencies and rates to the state variable indices
let mut state_variable_coefficients = vec![0.0; state_variable_indices.len()];
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, gotcha! this makes sense.

value default meaning
vehicle cost rate Raw use the observed values directly
coefficients 0.0 if a coef is not specified for a state variable, ignore the variable in the cost model

but we may want to add some error test here to protect against the case where someone forgets to populate a cost model in the TOML and the defaults result in zero-valued costs, like if all coefficients are zeroes at the end of this method -> return Err(_), and possibly the same for the rate values (if the user zeroes out all rate values with Factors).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we may want to add some error test here to protect against the case where someone forgets to populate a cost model in the TOML and the defaults result in zero-valued costs

yeah that's a great point, I'll add that in

@nreinicke
Copy link
Collaborator Author

did this change make a dent in your profiles?

it did reduce the search runtime a bit but nothing to write home about. I'm hoping that there are some other areas we haven't touched that might result in even bigger gains..

@nreinicke
Copy link
Collaborator Author

did this change make a dent in your profiles?

Actually, when I tested this out on a national run it had more of an effect. Before these changes I got these numbers for running batches of long queries over the national graph:

average route runtime: 137.328 seconds
average seconds per distance: 0.10788 seconds
average milliseconds per tree edge: 0.02720 milliseconds

And after

average route runtime: 33.852 seconds
average seconds per distance: 0.02219 seconds
average milliseconds per tree edge: 0.00545 milliseconds

@nreinicke nreinicke merged commit 0782939 into main Feb 6, 2024
5 checks passed
@nreinicke nreinicke deleted the ndr/cost-model-performance branch February 6, 2024 16:04
@robfitzgerald
Copy link
Collaborator

awesome! great stuff nick 👨‍🎤 🎸

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