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

Moved 'tot_tours' from nm tour frequency script to alternatives file #661

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

JoeJimFlood
Copy link
Contributor

To resolve #656

@jpn-- jpn-- added the Phase 8 Feature planned for inclusion in Phase 8 release label Jan 30, 2024
@jpn-- jpn-- self-assigned this Feb 6, 2024
@jpn-- jpn-- requested a review from dhensle February 6, 2024 19:24
@dhensle
Copy link
Contributor

dhensle commented Feb 6, 2024

I ran into an issue with this change awhile ago while doing some estimation work. Canonical_ids (used to create reproducible tour/trip ids) calculates IDs based on purposes specified in the alts file. If tot_tours is listed in the alt file, it is treated as another purpose. Thus, adding a column to input alts will change IDs and break estimation mode.

In other words, I believe this change should not pass the CI testing for estimation mode.

I think we have a couple of options:

  1. Explicitly specify the alts for canonical IDs in the example model(s) and keep the change as-is.
  2. Update canonical IDs to avoid tot_tours being counted as a separate purpose.
  3. Push this issue into data model dev task instead of phase 8. (could be done regardless of whether 1 or 2 is selected.)

@jpn--
Copy link
Member

jpn-- commented Feb 8, 2024

I chose option 2, which was relatively easy. A more comprehensive solution is deferred to whenever we address a "data model".

I ran into an issue with this change awhile ago while doing some estimation work. Canonical_ids (used to create reproducible tour/trip ids) calculates IDs based on purposes specified in the alts file. If tot_tours is listed in the alt file, it is treated as another purpose. Thus, adding a column to input alts will change IDs and break estimation mode.

In other words, I believe this change should not pass the CI testing for estimation mode.

I think we have a couple of options:

  1. Explicitly specify the alts for canonical IDs in the example model(s) and keep the change as-is.
  2. Update canonical IDs to avoid tot_tours being counted as a separate purpose.
  3. Push this issue into data model dev task instead of phase 8. (could be done regardless of whether 1 or 2 is selected.)

@jpn-- jpn-- merged commit 62c3523 into ActivitySim:develop Feb 8, 2024
18 checks passed
@jpn-- jpn-- mentioned this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phase 8 Feature planned for inclusion in Phase 8 release
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants