search

arup-group / pam

Generate and modify transport demand scenarios via a Python API.
MIT License
55 stars 21 forks source link

Return origin #237

Closed divyasharma-arup closed 1 year ago

divyasharma-arup commented 1 year ago

Fixes # TourPlanner now ensures all agents modify plans to return to origin. We had an issue where agents whose tour began close to the end of the day would not return to the origin before END_OF_DAY. The code specifically fixes the logic in TourPlanner.apply().

A test has been added to ensure this behaviour is corrected: test_end_of_day_agent_return_depot_by_end_of_day. To enable this test, the input test data had to be modified to allow for longer distances (and therefore a duration longer than a minute) to more easily support a test of an agent with a plan near end of day.

Some additional tests were added to increase coverage.

Checklist

Any checks which are not relevant to the PR can be pre-checked by the PR creator. All others should be checked by the reviewer(s). You can add extra checklist items here if required by the PR.

codecov-commenter commented 1 year ago

Codecov Report

Merging #237 (66e8678) into main (07813c8) will increase coverage by 0.70%. Report is 105 commits behind head on main. The diff coverage is 90.74%.

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   86.84%   87.55%   +0.70%     
==========================================
  Files          49       48       -1     
  Lines        5496     5728     +232     
  Branches     1372     1437      +65     
==========================================
+ Hits         4773     5015     +242     
+ Misses        462      442      -20     
- Partials      261      271      +10
Files Changed Coverage Δ
pam/activity.py 90.90% <ø> (+0.14%) :arrow_up:
pam/read/__init__.py 100.00% <ø> (ø)
pam/variables.py 100.00% <ø> (ø)
pam/write/__init__.py 100.00% <ø> (ø)
pam/write/matsim.py 76.08% <59.25%> (-2.58%) :arrow_down:
pam/core.py 88.85% <87.67%> (+0.82%) :arrow_up:
pam/read/matsim.py 87.57% <93.33%> (-0.85%) :arrow_down:
pam/vehicles.py 94.97% <94.97%> (ø)
pam/__init__.py 100.00% <100.00%> (ø)
pam/report/summary.py 93.12% <100.00%> (ø)
... and 1 more

... and 10 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

divyasharma-arup commented 1 year ago

What happens in the xtreme cases? For example;

(i) a departure from depot starts at 11.50pm and requests a 30 min trip to depot? (ii) same but for the return trip.

Is the plan completely discarded or do we get empty plans?

Hi Fred, originally, we would just have the agent stay at the origin if they did not have enough time to return to the origin. My proposed solution (for now) is to have them incrementally leave an hour earlier until they can make at least one trip.

You also suggested 'wrapping plans' to avoid this process of checking the plans beforehand. For certain populations (HGVs), I think wrapping plans could make sense, is there an existing example of this in PAM? For LGVs, however, it might make more sense to ensure they have plans that have them at the origin by end of day.

The problem with this approach of essentially editing the sampled plans is that the synthesised population may have what appear to be arbitrary deviations from the input samplers.

divyasharma-arup commented 1 year ago

Thanks all for the feedback, below is a summary of the problem, proposed solution for now, and changes in this branch:

Problem: The TourPlanner will return agents to origin, but this can possibly go beyond a single day. As agents add more stops (such as those with many stops), we are likely to find a fewer percentage of agents returning to their origin by end of day. This ranges from 15-25% not returning to origin, possibly higher if agents are given very long tours. The PAM fix_plans() method was only fixing the very last activity/leg, therefore the TourPlanner had to ensure only the last activity/leg was beyond a day. This has now been fixed per: https://github.com/arup-group/pam/issues/241 , and this logic of breaking an agents plan past a day is no longer required in TourPlanner.

Proposed Solution: We've discussed a few options here to address agents not returning to origin: ensuring each agent has a plan where they can return to the origin by end of day, 'wrapping' plans that go beyond a day (although we need to ensure plans cannot be more than 2 days), and introducing new specific TourPlanners. I'm proposing we go with the last one: a different type of TourPlanner that is based on a subpopulation's specific constraints. For example, a TourPlanner that has start/end/stops constraints can build a plan that meets these criteria. By distinguishing between types of agents that do tours (i.e., Service vs. Delivery tours), a TourPlanner that has plan constraints (i.e, must leave & return by a certain time, must do 18 stops) may be a more appropriate way to build plans vs. refining the inputs into the current TourPlanner to prevent multiple-day plans. For now, it's recommended that we do not have agents with a start time at the last hour of the day, as they are unlikely to have any additional legs/activities once the plan has been cropped by PAM.

Changes in this PR: Mainly a reduction in code:

If the above is satisfactory, we can merge this branch with its refactored code. I'll advocate for creating this new TourPlanner with input constraints as a way to add new subpopulations for a future project.

divyasharma-arup commented 1 year ago

Let me know of any further feedback @fredshone, @Theodore-Chatziioannou

brynpickering commented 1 year ago

@Theodore-Chatziioannou is this ready to merge (we can ignore the failing CI, that should sort itself out once this is merged into main)

Theodore-Chatziioannou commented 1 year ago

@Theodore-Chatziioannou is this ready to merge (we can ignore the failing CI, that should sort itself out once this is merged into main)

waiting confirmation by @divyasharma-arup