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

Current trip mode basing on current tour mode #3476

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

Conversation

dimaopen
Copy link
Collaborator

@dimaopen dimaopen commented Feb 25, 2022

Instead of setting mode for the whole tour (Home -> Work -> Shopping -> Home) and use it for each individual trip now we use current tour mode for getting the right current trip mode.
Also HouseholdFleetManager doesn't return a personal vehicle to persons that are not at home.


This change is Reviewable

@dimaopen dimaopen linked an issue Feb 25, 2022 that may be closed by this pull request
Copy link
Collaborator

@zneedell zneedell left a comment

Choose a reason for hiding this comment

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

Logic all looks good to me. I left one main suggestion for trying to make use of the Tour class that already extends PlanElement, which might give a more consistent way of keeping track of tour and trip modes.

For the record, my longer term plan/hope here is that we can eventually implement a tour mode choice that happens before trip mode choice in chooses mode. E.g. when someone comes to a leg in their plans without a tour mode set, they make an initial tour mode choice based on the skims looking at all of the trips in the tour and then set the tour mode there, then they can go about the individual trip mode choices in real time when they get to them. We could also improve the existing tour formation logic here to allow for nested subtours and/or reading in tour modes (potentially without trip modes) from activitysim.

BUT that's all down the road, I think this will get us 90+% of what we want.

@@ -475,6 +478,30 @@ class PersonAgent(
}
}

def isFirstTripWithinTour(personData: BasePersonData, nextAct: Activity): Boolean = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we could replace this check with instead checking if the current activity == home. I'm thinking of a case where someone did something like home -> work -> home -> shop -> home. We'd want them to be able to pick up their car for the second home -> shop -> home tour, for instance

Copy link
Collaborator

Choose a reason for hiding this comment

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

Later edit -- if we rely on the Tour class in BeamPlan this gets easier, I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tours are created basing on "Home" activity:

if (activity.getType.equalsIgnoreCase("home")) {

So I think it's generally the same (current activity == home and isFirstTripWithinTour).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That isFirstTripWithinTour method is not used anywhere and is kept for symmetry.

case _ =>
data.currentTourMode.orElse(modeOfNextLeg)
},
// We do not stick to current tour mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if here we can make use of the currentTour method defined above to take advantage of the tour structure already in BeamPlan.scala. It might take some archaeology to figure out how exactly everything was intended to function, but it looks like the strategies map in a BeamPlan allows for modes to be stored both for individual trips/legs and for tours.

So, I'm wondering if something like

val tourModeOption = _experiencedBeamPlan.getStrategy(currentTour(data), classOf[ModeChoiceStrategy])

would get the current tour mode, and we could do something similar with putStrategy when the initial trip mode is chosen and we want to set the tour mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replayed in the comment bellow.

@@ -587,15 +614,10 @@ class PersonAgent(
val nextCoord = nextActivity(data).get.getCoord
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I look at it, I wonder if the definition of modeOfNextLeg above can be replaced with

val modeOfNextLeg = _experiencedBeamPlan.getStrategy(nextAct, classOf[ModeChoiceStrategy])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replayed in the comment bellow.

@zneedell
Copy link
Collaborator

zneedell commented Mar 4, 2022

@dimaopen -- I played around with making some of the changes involving _experiencedBeamPlan in a new branch based off this one here: https://github.com/LBNL-UCB-STI/beam/tree/zn/do/%233442-current-trip-tour-mode.

I'd be curious to see if you think the logic there makes sense and aligns with what you were thinking

@Xuan-1998
Copy link
Collaborator

Test!

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.

Deferring to Zach's comments moreso. Mine are not blockers.

val replanningIsAvailable =
personData.numberOfReplanningAttempts < beamServices.beamConfig.beam.agentsim.agents.modalBehaviors.maximumNumberOfReplanningAttempts
(personData.currentTripMode, personData.currentTourMode) match {
case (_, Some(CAR | BIKE)) if personData.currentTourPersonalVehicle.isDefined => personData.currentTourMode
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 also a new behavior - as long as tour/trip aren't getting confused....I guess this raises the question of naming - should a different name be used so somebody doesn't accidentally use the wrong variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. @zneedell What do you think about currentTourMode vs currentTripMode names?

@dimaopen
Copy link
Collaborator Author

I'd be curious to see if you think the logic there makes sense and aligns with what you were thinking

Generally it looks good. Though we have some kind of duplication here: personData.(currentTripMode, currentTourMode) and _experiencedBeamPlan.getStrategy(currentTour/Trip, classOf[ModeChoiceStrategy]) needs to hold the same data (and also be updated in the right order).
I think that we need to remove one or the other.
By the way it already has some inconsistency. I did some sf-light runs using this new approach and you can see that a person forgot their car and used ridehail here:
car_ride_hail
If we look at a similar run with the old approach we can see that the tour mode is right:
car_choosen

Let me know what you think.

…rip-tour-mode

# Conflicts:
#	src/main/scala/beam/agentsim/agents/modalbehaviors/ChoosesMode.scala
@dimaopen
Copy link
Collaborator Author

@zneedell
I just found that the current PR also produces some inconsistent trips. You can see WALK in the middle of the trip.
walk_in_the_middle
Probably I need to fix this behaviour.

@Xuan-1998
Copy link
Collaborator

@zneedell
I just found that the current PR also produces some inconsistent trips. You can see WALK in the middle of the trip.
walk_in_the_middle
Probably I need to fix this behaviour.

Just out of curiosity here, so based on my understanding, we are forcing agents to use car when they depart with a car on the first Home trip? Why is this the case? I think it makes sense to me to let agents use other modes in the middle but just bring their car with them home. Correct me if I understand anything wrong here..

@zneedell
Copy link
Collaborator

@Xuan-1998 I think you're totally right that what you've described would be the ideal behavior. The logic to do it correctly is going to be a little tricky, though, and AFAIK it hasn't been implemented here yet. Basically, doing it as you describe I think would require scanning forward in an agent's plan to check whether the agent returns to the same place later in the day and then only allowing a non-car trip if we know they're going to be able to pick up their car later.

So I think that would be really cool if we could get that working, but I'd also be OK with implementing a half step here where we don't allow for that yet. But I do think that one of the potential benefits of going down the _experiencedBeamPlan.getStrategy route is that it might allow for this more complex logic

@zneedell
Copy link
Collaborator

zneedell commented Mar 23, 2022

OK I made some updates to https://github.com/LBNL-UCB-STI/beam/tree/zn/do/%233442-current-trip-tour-mode that seem to help (at least in my branch I had been saving the tripMode rather than tourMode to the tourMode column of events.csv, plus adding an explicit filter of the itineraries for mode choice based on the current tour mode). The columns here are the tour mode and the rows here are the actual mode taken:
image

(not sure where the walk trips with the car tour mode come from)

Agreed with Dima that we probably want to either go with this solution or doing it based on ChoosesModeData rather than trying to do both, and I'm definitely not convinced that this is the right way to do it, but I'll do a little more testing of it this week.

@dimaopen
Copy link
Collaborator Author

Test!

@dimaopen
Copy link
Collaborator Author

Test!

2 similar comments
@dimaopen
Copy link
Collaborator Author

Test!

@dimaopen
Copy link
Collaborator Author

Test!

@dimaopen
Copy link
Collaborator Author

Test now!

1 similar comment
@dimaopen
Copy link
Collaborator Author

Test now!

case data: ChoosesModeData
if data.personData.currentTourPersonalVehicle.isDefined &&
(
data.personData.currentTourMode.exists(mode => mode == CAR || mode == BIKE) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

This double exists against the same variable still is there - and is complicated by an &&. Can you add parentheses to delineate the intent please

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like it was re-added as part of de1684a

case firstVehicle :: rest =>
//in case of replanning because of TRANSIT failure WALK_TRANSIT is used
//but we may want to introduce maxWalkingDistance and check that the agent is close enough to the vehicle
case firstVehicle :: rest if atHome(originActivity) =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Providing a private vehicle only when the person is at home might be a wrong idea. It may lead to increased number of "Long waling trips".

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.

Walk issue in ChoosesMode
4 participants