-
Notifications
You must be signed in to change notification settings - Fork 100
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
Visualization Pull Request #532
Conversation
…be tested with csv inputs
…nitializing pipeline
… variables; added logging
…ed skim information that is lost right now.
simwrapper is on conda-forge now; remove pip install rules & docs
One comment I will make: I feel as though the way the aggregation maps are currently defined isn't super user-friendly. For example, the major access trip mode in the MTC example it's currently defined as follows:
While this can be directly input into the
This is less repetitive and more readable. Something like this would then need to be added before line 289 of summarize.py and 290 would need to be changed to use map instead of agg['map'] (I can make the change if needed):
|
#443: Visualization
#533: np.MachAr Deprecation (Hotfix)
Currently in DRAFT
Todo
Review Criteria Responses
Does it contain all the required elements, including a runnable example, documentation, and tests?
Yes. A runnable example is incorporated into
example_mtc
. Documentation has been updated to explain yaml and csv expression files used to configure the summarize model. New tests were created to check csv outputs from a minimal example.Does it implement good methods (i.e. is it consistent with good practices in travel modeling)?
Yes. The summarize model produces outputs in a flexible, industry-standard format (csv) and integrates fully into the Activitysim modeling pipeline. It does not impact travel modeling results, but operates on these results.
Are the runtimes reasonable and does it provide documentation justifying this claim?
Yes. The model adds only seconds to model runtime on a standard workstation.
Does it include non-Python code, such as C/C++? If so, does it compile on any OS and are compilation instructions included?
Yes and no. The changes to the ActivitySim package are completely implemented in Python, but it does add an additional dependency to SimWrapper.
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 AMPO Research Foundation, and, presumably, AMPO Research Foundation is providing the changes without any additional licensing beyond the existing ActivitySim licensing.
Does it appropriately interact with the data pipeline (i.e. it doesn't create new ways of managing data)?
The model ingests data from the pipeline and produces csv files in a standard "output" directory.
Does it include regression tests to enable checking that consistent results will be returned when updates are made to the framework?
The model does not impact modeling outputs. As such, no regression testing has been done.
Does it include sufficient test coverage and test data for existing and proposed features?
Yes. It includes a new test module with associated configuration files and input data.
Any other comments or suggestions for improving the developer experience?
No