-
Notifications
You must be signed in to change notification settings - Fork 57
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
switch from walk to EMERGENCY(faster walk) as last resort mode #3488 #3512
base: develop
Are you sure you want to change the base?
switch from walk to EMERGENCY(faster walk) as last resort mode #3488 #3512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a real pain to merge these changes only after #3497 is merged? Or it would be a hard time to validate that the Emergency mode works as expected.
Some(Right(TransitModes.TRANSIT)), | ||
TransportMode.other | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this EMERGENCY_TRANSIT
mode? Why we cannot use just EMERGENCY even if there is a TRANSIT leg in it?
@@ -115,7 +118,10 @@ object EmbodiedBeamTrip { | |||
theMode = leg.beamLeg.mode | |||
} else if (theMode == WALK && leg.beamLeg.mode == BIKE) { | |||
theMode = leg.beamLeg.mode | |||
} else if (theMode == WALK && leg.beamLeg.mode == EMERGENCY) { | |||
theMode = EMERGENCY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Because we introduce a new possible return value for
EmbodiedBeamTrip#tripClassifier
then we need to carefully check all the pattern matching on this field to validate that there wouldn't be aMatchError
. - Also because EMERGENCY trip is actually a WALK trip. Then we need to check all the places where
tripClassifer
is compared with WALK (i.e.if (trip.tripClassifier != WALK && trip.tripClassifier != WALK_TRANSIT) {
@@ -126,6 +132,8 @@ object EmbodiedBeamTrip { | |||
DRIVE_TRANSIT | |||
} else if (theMode == TRANSIT && hasUsedBike) { | |||
BIKE_TRANSIT | |||
} else if (theMode == TRANSIT && hasUsedEMERGENCY) { | |||
EMERGENCY_TRANSIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's safier not to change these method at all. But in this case we wouldn't have EMERGENCY
in PathTraversalEvent
.
src/main/scala/beam/agentsim/agents/modalbehaviors/ChoosesMode.scala
Outdated
Show resolved
Hide resolved
The actual emergency trip is a teleportation trip (as this task suggests) or a Walk trip? |
I was intending to change it to be a teleportation initially, but now I guess a faster Walk trip will be easier to validate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On top of what was already said, this double condition seems like a bug
@@ -261,7 +242,22 @@ object PersonAgent { | |||
bodyVehicleId: Id[BeamVehicle], | |||
bodyVehicleTypeId: Id[BeamVehicleType] | |||
): EmbodiedBeamTrip = { | |||
if (trip.tripClassifier != WALK && trip.tripClassifier != WALK_TRANSIT) { | |||
if (trip.tripClassifier != EMERGENCY && trip.tripClassifier != EMERGENCY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a copy/paste error or something to that extent - as the condition is the same
WIP |
This change is