Provide more flexibility for defining mandatory schedule specifications. #275
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#273. This change require defining tour scheduling specs specifically in the mandatory_tour_scheduling.yaml. The code still hard-codes the 'univ' value, and additional flexibility should be added in the future. This is step one of probably a couple of steps to unwind the hard-coded values in this section of the code.
Review Criteria Responses
Does it contain all the required elements, including a runnable example, documentation, and tests?
The example scripts have been updated so the examples and tests complete. It's unclear how the documentation should be updated for this change. Few of the internal workings of model settings are well-documented, and it seems like a large discussion (or work effort) is needed to standardize this across the models.
Does it implement good methods (i.e. is it consistent with good practices in travel modeling)?
Yes, this change improves the flexibility of the ActivitySim framework allowing it be implemented in more locations.
Are the runtimes reasonable and does it provide documentation justifying this claim?
This change will have no material change on the model run time. This code change replaces hard-coded values with a set of user-defined parameters.
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.
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.
Does it appropriately interact with the data pipeline (i.e. it doesn't create new ways of managing data)?
This change does not impact the data pipeline.
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.
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.
Any other comments or suggestions for improving the developer experience?
The documentation is still being built out, so it is difficult to understand where and how to add new documentation into the existing framework. Some documentation should be consistent across all models (as is the case with this change), so a standard would need to be set by the larger team or asserted by me.