-
Notifications
You must be signed in to change notification settings - Fork 99
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
Required Coefficients File Not Backwards Compatible or Documented #331
Comments
The COEFFICIENTS and COEFFICIENTS_TEMPLATE seem prone to user input error with all of the mappings between COEFFICIENT names, COEFFICIENT_TEMPLATE aliases, and actual usage in the the model specification or YAML file. Could the same logic in the COEFFECIENTS and COEFFICIENTS_TEMPLATE table be handled in a slightly more verbose single table? PROPOSED COEFFICIENT TABLE SCHEMA (used to map specific coefficient names to generic names)
In the table above, the segment name would be read and applied to the specific segment when available. The default segment would be used if no other segment matches and is assumed to be universal. This seems easier than the current implementation which requires keeping multiple files in sync. EXISTING COEFFICIENT TEMPLATE TABLE (used to map specific coefficient names to generic names)
EXISTING COEFFICIENT TABLE
(Admittedly, I've been out of the loop for a while, so I could clearly be missing the importance of this extra step too) |
This was implemented to support Larch, and the implementation is more clear. Closing this issue. |
Pull Request #325 introduced a new requirement that each model define a "coefficients" file. This makes the structure of the config files cleaner in the long run. However, the documentation has not been updated (as far as I can tell) to reflect this new requirement, and it breaks prior setups (e.g., ARC implementation) that does not abide by this standard.
activitysim/activitysim/core/simulate.py
Lines 138 to 140 in 05a25f6
We will update the ARC code base appropriately to follow the rules.
Might be worth discussing whether this new feature should truly be a requirement or whether it should just be strongly recommended.
The text was updated successfully, but these errors were encountered: