Skip to content

Conversation

@JoeJimFlood
Copy link
Contributor

To resolve #656

@jpn-- jpn-- added the Phase 8 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
@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

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants