School Escorting

[dhensle]

School drop-off and pick-up model implementation. Initial design details were presented at the March 8th meeting.

[coveralls]

Coverage remained the same at 0.0% when pulling 3ea0f4b on dhensle:school_escorting into f189143 on ActivitySim:develop.

[JoeJimFlood]

@dhensle I have finished my review. Here are my comments:

It might be good to add a check that NUM_ESCORTEES and NUM_CHAPERONES as defined in the settings file are consistent with the alternatives in school_escorting_alts.csv.

abm/models/school_escorting.py

• determine_escorting_par[t]icipants()
  • Line 40-41: Should model_settings.get() be used to prevent KeyErrors?
  • Line 47: Looks correct to me
  • Line 49/59: I agree with the comment that the driving age/escorting age should be configurable, especially if we want ActivitySim to be used outside of the US. If we do go in that direction should we have the mode choice spec files reflect that?
• create_school_escorting_bundles_table():
  • Line 215: Since the number of escortees is defined in the settings file, this should probably be for child_num in range(1, NUM_ESCORTEES+1) or something like that
  • Line 225: Just putting 0 here and removing Line 204 would make the code easier to read and more consistent with the field definitions before and after.

abm/models/non_mandatory_scheduling.py

• create_pure_school_escort_tours and assign_in_place are imported but never called (the latter is imported twice)

abm/models/non_mandatory_destination.py

• annotate is imported despite not being used

abm/models/non_mandatory_tour_frequency.py

• Is Line 7 needed given that Line 22 exists? No other functions from school_escort_tours_trips are called

abm/models/trip_purpose.py

• The function imported in Line 4 is not used

[dhensle]

@JoeJimFlood Thank you for your great comments. I have responded to each in bold below. Can you please review the changes and ensure they are satisfactory? Thanks!

It might be good to add a check that NUM_ESCORTEES and NUM_CHAPERONES as defined in the settings file are consistent with the alternatives in school_escorting_alts.csv.
Agreed, added a new check_alts_consistency method.

abm/models/school_escorting.py

• determine_escorting_par[t]icipants() fixed name typo
  • Line 40-41: Should model_settings.get() be used to prevent KeyErrors? yes, good catch
  • Line 47: Looks correct to me Thanks for checking. This was a comment to myself when developing. Removed comment.
  • Line 49/59: I agree with the comment that the driving age/escorting age should be configurable, especially if we want ActivitySim to be used outside of the US. If we do go in that direction should we have the mode choice spec files reflect that? added ability to specify options in the model yaml
• create_school_escorting_bundles_table():
  • Line 215: Since the number of escortees is defined in the settings file, this should probably be for child_num in range(1, NUM_ESCORTEES+1) or something like that yup, missed that one. Good catch.
  • Line 225: Just putting 0 here and removing Line 204 would make the code easier to read and more consistent with the field definitions before and after. I believe we actually do need to initialize before this and not just use 0. We do not want to overwrite what was set in the previous iterations of the loop. Left as is.

abm/models/non_mandatory_scheduling.py

• create_pure_school_escort_tours and assign_in_place are imported but never called (the latter is imported twice) Removed unused / re-imported references. assign_in_place is actually used on line 50, so I left that one but removed the second import.

abm/models/non_mandatory_destination.py

• annotate is imported despite not being used I see it used on line 119?

abm/models/non_mandatory_tour_frequency.py

• Is Line 7 needed given that Line 22 exists? No other functions from school_escort_tours_trips are called Nope, removed line 7.

abm/models/trip_purpose.py

• The function imported in Line 4 is not used Removed. No idea where this came from.

[JoeJimFlood]

@dhensle Everything looks good to me. Thank you so much for your work on this!

[jpn--]

I notice here that there appears to be an inconsistency between trip mode choice of chauffeur and escortee. Trip 242323629 is chauffeur escorting a child home from school, trip 242323981 is the child being escorted. Purpose, destination, schedule all line up as intended. But the chauffeur is driving (SHARED2FREE) while the child is using transit (WALK_LRF). Reviewing the various code changes, it looks like this discrepancy is not a bug in code that has been written, but just something that has not been locked down; I don't recall if this is an intentional or unintentional omission.

[dhensle]

Post-processing functionality has been added to force escortee tour and trip modes to match the chauffeur modes. For tour modes, there is ambiguity as the tour mode for the outbound leg can be different for the inbound leg since the pickup/dropoff can happen on different chauffeur tours. (The current implementation just uses the last available chauffeur tour mode. Maybe a mode heirarchy should be implemented? But this would have to change between modeling regions...) I am not sure it really matters considering the trip modes are made to match downstream.

[jpn--]

It looks like read_alts_file and read_spec_file are identical. Shall we use only one?

[dhensle]

Yes, only one is needed. And this was actually caught in the flexible trip and tour id's review. Resolved when merging in changes from develop branch.

[jpn--]

There's an error in the escorting spec here. dist_home1_to_mand isn't a valid variable, it should be dist_home_to_mand1. And again on the following line for "2". When running in normal simulation mode the error is avoided and our test can pass this module, as the coefficient on this line happens to be set to 0 in the associated coefficients file. But this will cause errors if run in estimation mode or with sharrow enabled, neither of which is currently tested for the school escorting model.

[dhensle]

Good catch. Fixed. It looks like the outbound and conditional outbound spec files did not have this error.