Conversation
There was a problem hiding this comment.
Pull request overview
Introduces new multimodal constraint capabilities based on traversal metrics (distance/time/energy) and leg-specific targeting, while moving trip-leg limiting into the MultimodalConstraintModel itself (closing #35).
Changes:
- Added energy state accessors and fieldnames for per-leg energy tracking.
- Expanded multimodal constraint configuration to include metric-based and leg-targeted constraints (distance/time/energy), plus a richer limit operation model.
- Moved max-trip-legs enforcement into
MultimodalConstraintModel::valid_frontierand updated tests accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/bambam/src/model/state/multimodal_state_ops.rs | Adds per-leg energy accessor to support energy-based constraints. |
| rust/bambam/src/model/state/fieldname.rs | Adds leg energy fieldname to standardize state variable naming. |
| rust/bambam/src/model/constraint/multimodal/model.rs | Enforces max trip legs early in valid_frontier; adjusts tests to remove explicit constraint. |
| rust/bambam/src/model/constraint/multimodal/mod.rs | Re-exports newly introduced constraint config types. |
| rust/bambam/src/model/constraint/multimodal/constraint_config.rs | Adds metric-based constraint types, leg targeting, and limit operations; updates config schema/docs. |
| rust/bambam/src/model/constraint/multimodal/constraint.rs | Implements validation for new metric-based constraints and conversion from config to runtime constraints. |
| rust/bambam-core/src/model/bambam_json.rs | Removes obsolete JSON model types. |
Comments suppressed due to low confidence (1)
rust/bambam/src/model/constraint/multimodal/constraint_config.rs:1
- These structs include both a
limitas auomquantity and a separateunit, but theunitis not used when evaluating constraints (testcomparesvaluedirectly toself.limit). As a result, configs like{ limit = 0.5, unit = \"miles\" }will either deserialize incorrectly (depending on howuomis configured) or will not apply the unit conversion. Consider storinglimitas a raw number (e.g.,f64) plusunit, and constructingLength/Time/Energyduring validation, or implement custom (de)serialization that converts(limit, unit)into the correctuomquantity.
use bambam_core::model::{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/bambam/src/model/constraint/multimodal/constraint_config.rs
Outdated
Show resolved
Hide resolved
yamilbknsu
left a comment
There was a problem hiding this comment.
Super cool. 🚀
I'm wondering if it would be a good idea for the future to extend the idea of the TripLegConstraint enum to be able to describe the mode_sequences.
Intrusive thought:
One could (not necessarily should) do something like:
mode_leg_time_limit.walk = { leg.type = "first", constraint = { limit = 0.0, unit = "minutes", op="min_exclusive" } }to force the first leg to be walk, right?
| /// mode_distance_limit.walk = { limit = 0.5, unit = "miles" } | ||
| /// ``` | ||
| /// | ||
| /// ## Walk mode can only be used at the beginning and end of a trip |
There was a problem hiding this comment.
Wouldn't this be Walk must be the beginning and end of a trip + Walk can only be used at the beginning and end of a trip?
And if I'm reading it correctly, it should be exact_sequences right?
There was a problem hiding this comment.
ah good catch. rewrote it to say Walk mode can only be used in the first or third leg of a trip which may include a middle leg in either bike or drive mode. and changed to exact_sequences
maybe? it would require refactoring the
that should work! maybe something we circle back around to if we encounter issues with performance. |
this PR Closes #35 by introducing additional multimodal constraints that are based on metrics rather than the mode switching history of the trip.
note: this implementation is complete for distance and time metrics, but not for energy. a tech debt issue was created for this in #121, since energy-based metrics are not currently a priority.
along the way,
NonZeroU64to prevent underflow/prevent invalid configuration (max == 0 is nonsensical)