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

[Bug -- specification -- minor] Persons selecting work_from_home not excluded from telecommuting frequency #627

Closed
DavidOry opened this issue Dec 8, 2022 · 8 comments

Comments

@DavidOry
Copy link

DavidOry commented Dec 8, 2022

Describe the bug

As discussed in #609, a person "working from home" in ActivitySim does not have a usual work place and never commutes in the model. In the current SEMCOG prototype, persons selecting to work from home in the work from home module are then subjected to the telecommuting frequency module. This is inconsistent with the ActivitySim definitions: persons working from home never telecommute because they do not have a usual work location.

The implications of the bug should be minor. The primary impact of the telecommute frequency module is on the downstream coordinated daily activity pattern (CDAP) module. But those working from home are properly excluded from selecting a mandatory pattern in CDAP. Because those working from home have erroneously received a telecommuting frequency outcome, the coefficients specific to these outcomes in CDAP will be misapplied, slightly changing the non-mandatory and home patterns for those working from home. A very small amount of unnecessary computation is also being done due to the error.

The main reason to fix the error is that it is confusing to users. As discussed in #609, the definitions related to telecommute and work from home are already challenging to keep straight. The model outcomes should reinforce these definitions, rather than contradict them.

To Reproduce

This error was discovered by our colleagues in Australia who are applying the work from home and telecommuting model to an ActivitySim prototype in Melbourne (thanks Andrea Colaiacomo). It can be reproduced by doing a cross tabulation of the telecommute frequency and work from home outcomes.

Failing tests

There are no tests for this error and probably do not need to be.

Thoughts on resolution

Adding @jfdman, @aletzdy, @dhensle, and @AndrewTheTM to discuss resolution.

Adding a CHOOSER_FILTER_COLUMN_NAME to the telecommute_frequency.yaml file to exclude those with a work_from_home value of true from the telecommute frequency model may be the cleanest solution.

Environment

N/A

@AndrewTheTM
Copy link
Contributor

In MWCOG's current implementation (which is ahead of the prototype) this shouldn't be happening. The work zone id for wfh is being set to -1 in https://github.com/MWCOG/Gen3_Model/blob/41ec6ab184eb205e467d665ddc26688dd77fe0a2/source/scripts/python/extensions/work_from_home.py#L98 and the telecommute frequency model requires a work zone id that is greater than -1 (see https://github.com/MWCOG/Gen3_Model/blob/41ec6ab184eb205e467d665ddc26688dd77fe0a2/source/scripts/python/extensions/telecommute_frequency.py#L38).

This is at least an effective workaround to the issue.

@DavidOry
Copy link
Author

DavidOry commented Dec 8, 2022

In MWCOG's current implementation (which is ahead of the prototype) this shouldn't be happening. The work zone id for wfh is being set to -1 in https://github.com/MWCOG/Gen3_Model/blob/41ec6ab184eb205e467d665ddc26688dd77fe0a2/source/scripts/python/extensions/work_from_home.py#L98 and the telecommute frequency model requires a work zone id that is greater than -1 (see https://github.com/MWCOG/Gen3_Model/blob/41ec6ab184eb205e467d665ddc26688dd77fe0a2/source/scripts/python/extensions/telecommute_frequency.py#L38).

This is at least an effective workaround to the issue.

Thanks Andrew.

  1. Do you think it's better to have the filter in the code or in a yaml/csv file that is exposed to the user?
  2. I cannot access the links. Are they private? Is the intent to do a PR for the telecommute_frequency.py modification back to the Consortium?

@JilanChen
Copy link

SEMCOG also noticed this issue in SEMCOG's latest version and brought it to RSG's attention. There are around 6% of "Work-from-home" records with telecommuting options and less than 1% with num_work_tours not 0. I agree with David this should be fixed to avoid the confusion though the impact could be small in the results.

@AndrewTheTM
Copy link
Contributor

@DavidOry I just updated a branch for the MWCOG prototype, see https://github.com/AndrewTheTM/activitysim/blob/develop_mwcog_prototype/activitysim/examples/prototype_mwcog/extensions/work_from_home.py#L98 and https://github.com/AndrewTheTM/activitysim/blob/259c49042c9c5217e67667bb3f1dd37d1c0e19a4/activitysim/examples/prototype_mwcog/extensions/telecommute_frequency.py#L38, they should be visible to everyone. These will eventually make it back into the repository as part of the MWCOG prototype.

Regarding point 1, with the workarounds in place I don't know if it's a critical need to do now (although it's not difficult to make the change either). But if a model has something related to job type or something that can be used to turn on/turn off WFH (splitting the types of workers that can and cannot work from home), then I think we'd ultimately want this in the yaml file.

@DavidOry
Copy link
Author

DavidOry commented Dec 8, 2022

Got it, thanks @AndrewTheTM. Yes, I agree the timing here is not critical, but I do think it's useful to agree on the broader convention: when we have a filter, is our first choice to put it (a) in the code, (b) in the model-specific yaml, or (c) the model-specific csv. If we agree, then we can, as is convenient, modify the implementations of the telecommute frequency models to be consistent, which makes it easier for new adopters.

Others: anyone disagree with putting this type of filter in the yaml when possible?

@aletzdy
Copy link
Contributor

aletzdy commented Dec 8, 2022

The Work_from_home.py script is supposed to overwrite the workplace_zone_id of those who work from home with -1, as @AndrewTheTM's first shared link shows, but we found a bug in the script that prevented that, and fixed it (in short, we should take out the is True from this line. This bug generally affected models where work_from_home is run after workplace_location.

As Andrew also mentioned, the TC model only runs on a sample of persons with workplace_zone_id>-1, so with this fix, we should be fine.

@colaandrea
Copy link

colaandrea commented Dec 12, 2022

Hi @aletzdy ,
I see the line you are referring to, only applies if the column exists. So solely relying on it wouldn't be enough to ensure people working from home won't be able to telecommute.

On top of it, there is nothing dealing with workplace_zone_id in my version of the code...I'll try to understand why.

For now I'll try to implement the filtering on is_out_of_home_worker.

@dhensle
Copy link
Contributor

dhensle commented Feb 2, 2023

This was addressed in ActivitySim v1.2 release.

@dhensle dhensle closed this as completed Feb 2, 2023
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

No branches or pull requests

6 participants