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 3ea0f4b8ea9386379cf15c5ccef275f80c8e7b70 on dhensle:school_escorting into f1891430d2222b8be8212de1cef498bd75ac66b2 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!