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

switch from walk to EMERGENCY(faster walk) as last resort mode #3488 #3512

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

Xuan-1998
Copy link
Collaborator

@Xuan-1998 Xuan-1998 commented Apr 5, 2022

This change is Reviewable

@Xuan-1998 Xuan-1998 self-assigned this Apr 5, 2022
Copy link
Collaborator

@dimaopen dimaopen left a 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
)

Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 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 a MatchError.
  2. 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) {
    ) and add these EMERGENCY there.

@@ -126,6 +132,8 @@ object EmbodiedBeamTrip {
DRIVE_TRANSIT
} else if (theMode == TRANSIT && hasUsedBike) {
BIKE_TRANSIT
} else if (theMode == TRANSIT && hasUsedEMERGENCY) {
EMERGENCY_TRANSIT
Copy link
Collaborator

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.

@dimaopen
Copy link
Collaborator

dimaopen commented Apr 5, 2022

The actual emergency trip is a teleportation trip (as this task suggests) or a Walk trip?

@Xuan-1998 Xuan-1998 changed the title switch from walk to teleportation as last resort mode #3488 switch from walk to EMERGENCY(faster walk) as last resort mode #3488 Apr 5, 2022
@Xuan-1998
Copy link
Collaborator Author

Xuan-1998 commented Apr 5, 2022

The actual emergency trip is a teleportation trip (as this task suggests) or a Walk trip?

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

Copy link
Collaborator

@JustinPihony JustinPihony left a 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) {
Copy link
Collaborator

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

@Xuan-1998
Copy link
Collaborator Author

WIP

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

Successfully merging this pull request may close these issues.

3 participants