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

Write Number of Participants to Trip List Output #372

Closed
wants to merge 2 commits into from

Conversation

danielsclint
Copy link

As a convenience method, the ARC Cube scripts need a way to expand the output trip lists based on the number of participants. This pull request adds an additional field "number_of_participants" to the final_trips.csv. It is a mirror of the similar value on the final_tours.csv.

This PR also removes a vestigal time_period_label method that had been replaced the new Network LOS class.

Review Criteria Responses

  1. Does it contain all the required elements, including a runnable example, documentation, and tests?
    The examples are updated with the appropriate configuration changes. No new documentation is necessary. No new tests were created, but the existing tests all pass.

  2. Does it implement good methods (i.e. is it consistent with good practices in travel modeling)?
    Yes, it provides more flexibility to support trip table expansion without necessitating table joins. This extends the spirit of the ActivitySim project to provide maximum implementation flexibility while maintaining a common workflow and model specification.

  3. Are the runtimes reasonable and does it provide documentation justifying this claim?
    This change has no material impact on runtimes.

  4. Does it include non-Python code, such as C/C++? If so, does it compile on any OS and are compilation instructions included?
    No. This is a Python-only change.

  5. Is it licensed with the ActivitySim license that allows the code to be freely distributed and modified and includes attribution so that the ‘provenance’ of the code can be tracked? Does it include an official release of ownership from the funding agency if applicable?
    This work was done under contract to ARC, and, presumably, ARC is providing the changes without any additional licensing beyond the existing ActivitySim licensing.

  6. Does it appropriately interact with the data pipeline (i.e. it doesn't create new ways of managing data)?
    This change requires a change to the data pipeline to deal with redundant field names in downstream models. The examples have been updated.

  7. Does it include regression tests to enable checking that consistent results will be returned when updates are made to the framework?
    No regression testing has been done. The code change removes a hard-coded value in the Python code with a set of user-defined variables. If the user specifies the same values as the previously hard-coded values, they should get the same results. The unit tests seem to confirm this assertion.

  8. Does it include sufficient test coverage and test data for existing and proposed features?
    The test configuration files were modified to use the newest features.

  9. Any other comments or suggestions for improving the developer experience?
    No. This is pretty straightforward.

@guyrousseau
Copy link

This PR also removes a vestigial ...

@coveralls
Copy link

coveralls commented Jan 22, 2021

Coverage Status

Coverage increased (+2.3%) to 80.318% when pulling 9020abf on danielsclint:ft_matrices into 2ef3bb0 on ActivitySim:develop.

@bstabler
Copy link
Contributor

bstabler commented Feb 3, 2021

thanks @danielsclint. The pipeline table joint_tour_participants contains the number of participants and can be output via the settings file output_tables list and then table joined later if needed. I think this is a more elegant solution than adding REDUNDANT_TOURS_MERGED_CHOOSER_COLUMNS directives to submodels to deal with additional data we added upstream but don't need until we're using the outputs in an external process. Thoughts?

@bstabler
Copy link
Contributor

bstabler commented Feb 4, 2021

  • @JilanChen suggested adding the the field you suggest as a good way to help document what's in the trips table.
  • @joecastiglione suggested we make it configurable to write different types of trips tables - raw trips, expanded person level trips, etc.

@bstabler
Copy link
Contributor

Another idea discussed is to improve the output trip writer to support annotations as a way to support more flexible/custom output writing

@danielsclint danielsclint deleted the ft_matrices branch May 10, 2021 23:04
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.

4 participants