8000 Rjf/isolate traversal models by robfitzgerald · Pull Request #318 · NREL/routee-compass · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rjf/isolate traversal models #318

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 100 commits into
base: main
Choose a base branch
from
Open

Rjf/isolate traversal models #318

wants to merge 100 commits into from

Conversation

robfitzgerald
Copy link
Collaborator
@robfitzgerald robfitzgerald commented May 8, 2025 •
edited
Loading

this big old change improves traversal modeling on Compass so that each TraversalModel updates a single dimension of the traversal, as in the single responsibility principle. API changes and refactored modules resulted in far-reaching changes.

todo

  • test on notebooks
  • python e2e test on denver
  • store default feature names in a single module
  • update documentation

changed API + implementation for traversal modeling

topological dependency sorting and validation

topological sorting added via the topological_sort crate. that is used to sort the configured models. this means the user's config does not need to guarantee correct model order. this is done once when constructing a CombinedTraversalModelService, see combined_ops.rs

gotchas

  1. must include a dummy grade model in the configuration when using the energy model
  2. traversal summary includes "edge_*" features which may be confusing

future work

@robfitzgerald
add state feature types
1038d06
@robfitzgerald
expand derivations + implementations
22fe054
@robfitzgerald
cleanup
bf46908
@robfitzgerald
revised ICE vehicle modeling
0896f59
@robfitzgerald
prep for removal
75a3b73
@robfitzgerald
revised distance traversal modeling
c340416
@robfitzgerald
revised siloed traversal modeling
7c1cca4
@robfitzgerald
rename constant
b17f874
@robfitzgerald
add state feature types
23da780
@robfitzgerald
revised ICE vehicle modeling
7e28b08
@robfitzgerald
improve APIs
36b9d89
@robfitzgerald
stub out remaining traversal models
ed1760b
@robfitzgerald
clippy
88c5df3
@robfitzgerald
bad edit
375daa5
@robfitzgerald
grade model
1ab1d4d
@robfitzgerald
speed model
fb97802
@robfitzgerald
speed model
f6e5a1c
@robfitzgerald
distance model
6289413
@robfitzgerald
rename models + namespaces
6e4af49
@robfitzgerald
input/output state features
4c44a2e
@robfitzgerald
clippy
43fb01e
@robfitzgerald
changed semantics in tests
b79f1f5
@robfitzgerald
add input features to StateModel.register
6134755
@robfitzgerald
handle input/output features
3aba391
@robfitzgerald
combined traversal model
a6209ab
@robfitzgerald
clippy
1697c4f
@robfitzgerald
Merge branch 'main' into rjf/isolate-traversal-models
f2c3e93
@robfitzgerald
Merge branch 'main' into rjf/isolate-traversal-models
a7aecbb
@robfitzgerald
From OutputFeature
a2b7293
@robfitzgerald
include variable name in invalid index error
8690e92
@robfitzgerald
Copy link
Collaborator Author

@nreinicke i've also replaced the python test (python/tests/test_downtown_denver_example.py) with the monotonicity test. the previous test was only confirming that "app.run()" did not fail, so, that is included as part of the new test.

@nreinicke
Copy link
Collaborator

Thanks for adding that new monotonic test in! When I pull these changes down and run the 03_time_energy_tradeoff_example.py, I'm getting some weird results (after fixing the example to run with the new code). Here's what I'm seeing for the battery electric time energy tradeoff chart:

image

Are you able to reproduce this on your side? I'm surprised to see this since the montonic test is passing.

@nreinicke
Copy link
Collaborator

Also - I'm not sure I understand the difference between the "trip_energy_electric" weight and the "edge_energy_electric" weight? If I change the monotonic test to use "trip_energy_electric" rather than "edge_energy_electric", the test fails.

@robfitzgerald
Copy link
Collaborator Author

Also - I'm not sure I understand the difference between the "trip_energy_electric" weight and the "edge_energy_electric" weight? If I change the monotonic test to use "trip_energy_electric" rather than "edge_energy_electric", the test fails.

OH so glad you caught that. i did drop a note here about the naming convention. edge_* is the value for just the edge, and trip_* is the value for the entire trip. we should be looking at trip_*. i've updated the test, and now i'll look into what's going wrong here.

@robfitzgerald
Copy link
Collaborator Author

i'm seeing non-determinism again now. sometimes cost-optimal fails to be less than either energy optimal or time optimal. i'm guessing it's a mix-up somewhere in either 1) recording trip_, edge_ state correctly or 2) using trip_ vs edge_ attributes in the cost model. will investigate.

AssertionError: 2.371698397370439 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost
AssertionError: 2.371698397370439 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost
AssertionError: 2.3716983973704395 not less than or equal to 2.371698397370439 : not balanced.cost <= time.cost
AssertionError: 2.371698397370439 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost
AssertionError: 2.3716983973704395 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost
AssertionError: 2.371698397370439 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost
AssertionError: 2.3716983973704395 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost
AssertionError: 2.371698397370439 not less than or equal to 2.3671130603324873 : not balanced.cost <= energy.cost

Sign up for free to join this conversation on GitHub. Alre 6AD3 ady have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@nreinicke nreinicke Awaiting requested review from nreinicke

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robfitzgerald @nreinicke
0