-
Notifications
You must be signed in to change notification settings - Fork 19
Example Review
Replace VERSPM modules with PSU’s Multi-Modal model, including modules which calculate household average daily vehicle miles traveled (ADVMT), and household average daily trip and miles by each of 3 alternative modes: transit, walk, bike.
VETravelDemandMM Contribution Pull Request, including responses to the review criteria. The Repository Manager and the Review Team Chair also met with the Contributor ahead of time to answer questions.
Review Question | Ben’s Comments | Tara’s Comments |
---|---|---|
1. Does it contain all the elements that are required by the VisionEval system specifications? | yes, it is modular, open, includes estimation capabilities, good documentation, tests, and it runs fast | Ditto. Good coverage of System Specs Section 8 on modules. |
2. Why is it better, and/or different than existing modules? Does it do good science and provide documentation justifying this claim? Is it consistent with good practice in strategic modeling? How might it overlap with existing modules? | better policy sensitivities for non-driving modes and taking advantage of newer and better data sources available since the implementation of the RSPM/GreenSTEP model. | More recent data, more rigorous, explicit built form 5D variables. 5D variables pulls together RPAT and RSPM functionalites. ODOT-led peer-reviewed research process(SPR#788). Provides opportunities to improve existing modules with new 5D attributes. Would like to see Flow chart/how changes fit into existing model system. Cheat sheet of estimated functions & included variables.. |
3. Is the module documentation complete? Does it include documentation of model estimation, algorithms, and instructions for using? | yes, it has vignettes and excellent inline code documentation | yes. Some issues with estimation data confidentiality (excludes external packages NHTS and SLD), although estimation scripts/documentation otherwise complete. |
4. If the module allows the estimation of regional parameters, does it provide default data, does it have clear documentation of what the estimation data needs to be and how it is to be formatted, and does it include proper data specifications to ensure that the user’s input data are correct? | yes, including data, functions, and documentation; the estimation procedures are a bit different than VE's | Changes to inputs may make it harder to use the tools. NOTE; Discussion clarified that this refers to regional estimation, which is not applicable to the submitted modules. |
5. Is it based on geographic definitions that are consistent with the model system definitions? | Yes, and it uses the RVCOG test example | yes. |
6. Does the module compute quickly enough and provide documentation justifying this claim? | yes, run times are a couple minutes for the RVCOG example | Do we need to test full model? Have bigger model test datasets? |
7. Does it includes all source files and data? If a contributed module does not include all source data, it should include a minimal example data file for testing and so it is clear what data structure is needed to run the module. It should also include clear instructions on how to fetch the data and/or a clear explanation of why non-included data is confidential and contact information for data owners. | yes, but it has confidential data, which is important to note. It does include all the other data as R data files. Dependent packages (NHTS2009, SLD) are not included in the submission. | Data confidentiality issues need to be addressed to enable authorized access to estimation datasets. |
8. Does the module only call R code and packages that work on all operating systems? If the code includes any non-R code (e.g. FORTRAN, C++) will that code compile on all operating systems? | only R code | |
9. Is it licensed with the VisionEval license that allows the code to be freely distributed and modified and includes attribution so that the ‘provenance’ of the code can be tracked? | it uses the VE license | Need to discuss open potential issue of ODOT code ownership |
10. Does it only interact with the computing environment by returning a properly structured list to the framework (i.e. it does not modify the global environment, does not read or write files, and only calls framework functions that are allowed)? | yes, it uses the VE lists and does not misuse the global environment | Selected package functions fit better in VESynHH module. |
11. Does it include regression tests to enable checking that consistent results will be returned when updates are made to the framework and/or R programming environment? | Yes, the checkModuleOutputs function in testModule checks test outputs | |
12. Does it include sufficient test coverage and test data? Does it pass the ‘testModule’ test which validates that it will run correctly in the model system? | yes, it has good test coverage and is already passing Travis tests. It does devtools::check(), build and installation with R CMD INSTALL, and package tests in tests/scripts/test.R (with the RVCOG example). | Sensitivity testing at different levels with comparison to literature elasticities (module, RVMPO model results, VE integration) |
Ben: Submission looks great and is quite complete. Travis returns a bunch of R package warnings that should be addressed, such as ‘DoPredictions: no visible binding for global variable ‘model’. The dependent data packages (NHTS2009 and SLD) should be submitted. The overall VE project travis.yaml script should be updated to include the new module and dependent packages in the automatic testing. The estimation methods should be revised to follow VE conventions. Some of these suggested revisions need to be discussed with the project team since we’re still figuring out how we all work together and incorporate additions.
Tara: Submission has a more rigorous output estimation of non-vehicle travel (bike, walk, transit) than the current VE module. In addition the model of household ADVMT has better sensitivity to auto ownership; which is non-linear. This is important in car sharing and automated vehicle policies. It also includes a new household drivers and auto ownership funtions.. Addition of built form variables provides opportunities for improving other modules (e.g., effectiveness of TDM depends on built form/5Ds of home location) and brings RPAT and RPSM functionalities together. It went through ODOT-led peer-reviewed research process (SPR#788) that included engagement by FHWA, transit, planning, and regional MPO technical staff at various decision points. Package includes several levels of sensitivity testing (module, model, and literature elasticity). New inputs are required but appear to be some start at US-wide tools (e.g., Placetypes_USA, using 2010 EPA SLD data) to assist in their development.
As a result, we recommend accepting the submission after addressing the issues noted:
- update travis automated testing script to test new package
- revise the documentation/software to let the user know that the NHTS2009, SLD, confidential data for estimation, and estimation script are exceptions to the guidelines for various reasons
- add proof of ODOT release of ownership
- vignette and/or cheat sheet summarizing estimated functions and dependent variables
Status | Software | Documentation | Methods |
---|---|---|---|
Accept | x | x | |
Accept but recommend revisions | x | ||
Do not accept | |||
Abstain |
- Getting Started
- VisionEval Models
- VERPAT Tutorial
- VERSPM Tutorial
- VE-RSPM Training
- Developer Orientation
- Goals and Objectives
- Working Together
- Automated Testing
- Contribution Review Criteria
- Modules and Packages
- Development Roadmap
- Documentation Plan
- Multiple Scenarios