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

Pt interchange duration #220

Merged
merged 5 commits into from
Aug 23, 2022
Merged

Conversation

Theodore-Chatziioannou
Copy link
Contributor

In a recent simulation, we noticed that the "pt interaction" activity can have duration > 0. The plan handlers are ignoring those activities (see here), and as a result the start times of the next leg end up wrong (since the activity end time does not get updated.

We are not sure that this "pt interaction" activity represents in these cases.

I am trying to fix the timestamp bug by checking for an "end_time" tag in the "pt interaction" activity. If there is one, then pt interaction is treated as a normal activity, separating legs/trips either side.

@@ -739,7 +739,7 @@ def process_plans(self, elem):
if stage.tag == 'activity':
act_type = stage.get('type')

if not act_type == 'pt interaction':
if not (act_type == 'pt interaction' and (stage.get('end_time') is None)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will create new trips where a pt interaction is encountered with an end_time. This seems wrong.

Instead i think we need to simply allow for the fact that a pt_interaction can have a duration?

ie:

if stage.tag == "activity":
    # update time
   if not act_type == 'pt interaction':
       # logic for new trip

Copy link
Collaborator

Choose a reason for hiding this comment

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

prior logic seems to assume duration of pt interactions is 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the sequence to :

if stage.tag == "activity":
   if not act_type == 'pt interaction':
       # logic for new trip
   elif stage.get('end_time'):
       # update time
   else:
      # zero pt-interaction duration

@andkay
Copy link
Contributor

andkay commented Aug 19, 2022

Agreed with @fredshone.

Also, if we are explicitly handling cases where pt-interaction does or does not include an end_time, we should include tests for both.

@Theodore-Chatziioannou
Copy link
Contributor Author

you are right, the pt interaction activity should not separate leg of trips, and only update the timestamps. I have corrected for this, and added tests for both zero-duration pt interactions and non-zero ones.

@andkay
Copy link
Contributor

andkay commented Aug 22, 2022

Looks good to me @Theodore-Chatziioannou. I don't know how much capacity we have for this at the moment. Time permitting, a larger refactor on these handlers would be a good idea. Process plans is doing quite a lot of work and the control flow is pretty difficult to follow.

@Theodore-Chatziioannou
Copy link
Contributor Author

I agree @akay-arup - added it as issue #221 .

@Theodore-Chatziioannou Theodore-Chatziioannou merged commit 42d04ba into main Aug 23, 2022
@arup-group arup-group deleted a comment from divyasharma-arup Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants