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

make activity loader use first activity type, not assume home #52

Closed
wants to merge 1 commit into from

Conversation

gac55
Copy link

@gac55 gac55 commented Nov 11, 2020

This makes the load_activity_plan method consistent with other methods, where it is not assumed the first activity is a home type.

Came across this when using activity_plan type with freight, delivery plans.

@gac55 gac55 requested a review from fredshone November 11, 2020 15:00
@gac55
Copy link
Author

gac55 commented Nov 11, 2020

I will make a change to the test

@gac55
Copy link
Author

gac55 commented Nov 11, 2020

I am confused by these tests. As an example, consider the inputted activity plans for these tests.
You can see agent pid=0 has the following activities:

pid | hid | seq | hzone | ozone | dzone | activity | mode | tst | tet | freq
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
  | 0 | 0 | 1 | a | a | b | work | car | 500 | 530 | 10
  | 0 | 0 | 2 | a | b | a | home | car | 1000 | 1030 | 10

i.e. work-home but the test is looking to assert home-work-home here

My change stops the force change of the first activity to home, resulting in a failed test. Have I misunderstood the test here @fredshone or @mfitz ?

I can change the test to assert work-home or change the inputted test csv if I have understood correctly.

@gac55 gac55 requested a review from mfitz November 11, 2020 16:12
@fredshone
Copy link
Collaborator

fredshone commented Nov 11, 2020

There is some work do be done in clarifying the various read methods.

I think the one you are using was explicitly added for the 'new' (April) TFL data which were home based tours. From the load_activity_plan docstring:

 Turn Activity Plan tabular data inputs (derived from travel survey and attributes) into core population
    format. This is a variation of the standard load_travel_diary() method because it does not require
    activity inference. However all plans are expected to be tour based, so assumed to start and end at home.
    We expect broadly the same data schema except rather than trip 'purpose' we use trips 'activity'.

In which case the test makes sense I think as the first activity is home and was required to be 'inferred' as it was not in the input data (as per the test). If this is not the case for the data be read then the read method can be adjusted. I would propose adding an option, eg force_home_base = True.

@gac55
Copy link
Author

gac55 commented Nov 11, 2020

Ok - understand, makes sense. I will implement a flag as you suggest.

Copy link
Collaborator

@fredshone fredshone left a comment

Choose a reason for hiding this comment

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

see comment, prefer adding a flag as suggested.

@gac55
Copy link
Author

gac55 commented Jan 26, 2021

Raised as an issue here

@gac55 gac55 deleted the gc-update-first-activity branch January 26, 2021 14:42
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.

None yet

2 participants