Check Settings, Spec, and Coefficients Files #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… add second model to validate
… settings to independent function
…nction called from main checker. run formatter
jpn--
reviewed
Mar 24, 2025
|
|
||
| def check_model_settings(state: State) -> None: | ||
|
|
||
| components = state.settings.models # _RUNNABLE_STEPS.keys() may be better? |
Collaborator
There was a problem hiding this comment.
state.settings.models is probably the right thing here not _RUNNABLE_STEPS, as the latter includes every step that might be run, but the former includes only that which we want to run right now. It's totally legit to have incomplete/invalid settings for steps we are not trying to run right now.
| logger = logging.getLogger(__name__) | ||
| file_logger = logger.getChild("logfile") | ||
|
|
||
| COMPONENTS_TO_SETTINGS = { |
Collaborator
There was a problem hiding this comment.
This is probably a necessary evil for now.
…n empty dataframe, even if no top level spec exists
…or disaggregate accessibility (needs more testing)
This reverts commit be5d093.
…in model settings
missing values. because the fields in the settings classes are inherited, it is not usually possible for the settings checker to determine if a path to an external file is actually required for a particular model component. the workaround is to issue a specific warning that a path *may* be expected and users should check the YAML file. the ultimate solution would be to define more robust Pydantic data models that ensure that fields are marked as required when appropriate.
…s to checker entrypoint as argument.
* initial batch of models with pre and post processing added * nonmand sched and transit pass models * first pass at preprocessing and annotate functionality in all models * fixing bugs in jtf and trip purpose * adding persons back in to locals_d in jtc * model name missing in tour scheduling * missing expressions import in tour sched prob * ci unit tests & fixing estimation test error * addressing review comments --------- Co-authored-by: Ali Etezady <[email protected]>
* first uv sync * explicitly set np.int64 dtype before calling pd set_index() Pandas 2 got rid of the `Int64Index` class, it was downcasting to int32 for the input land use table * Add all dependencies from Conda environments Consider removing unused ones at a later time * Remove orca This dependency was replaced with “state” for workflow orchestration. * Update release instructions for `uv` * Remove `--no-default-groups` from `uv sync` instructions * Trial updating Github Action to uv instead of conda * Bug fixes * Try downgrading sharrow * change core test to windows runner * use win for all core tests * index type mismatch * formatting * unlock sharrow to 2.14 * correct a typo * Update Model Setup page in user guide * Update Ways to Run the Model page * Remove duplicates * Simplify pyproject.toml for github action and use only this group * Update remainder of user guide docs * Update dev guide [makedocs] * Update lockfile from changes to dependencies * Remove conda from other github actions * Debug doc building [makedocs] * Update install instructions [makedocs] * update installation instructions [makedocs] * notes on uv options [makedocs] * address review comment --------- Co-authored-by: Josie Kressner <[email protected]>
* identify and flag naming conflicts in 2d vs 3d skims * add skims doc * run on workflow_dispatch * add link to docs * write error message in exception also * add error message example * code block formatting * unit tests
…g.yaml from MWCOG example model
* update cat columns in df when new cat exists * blacken * sort two categoricals before union
* minor fixes for SimOR development * blacken * Using PNUM if available in JTFC Required to maintain backwards compatibility in tour indexes in SANDAG models --------- Co-authored-by: Jeffrey Newman <[email protected]>
…activitysim version to logfile (ActivitySim#963) * adding activitysim version to log file * fix duplicate columns in parking location choice as described in ActivitySim#633 * making tour mode choice logsum optional in destionation choice models ActivitySim#847 * logsum settings in location choice, trip dest, tour_od * optional logsums in trip destination * blacken --------- Co-authored-by: Ali Etezady <[email protected]>
Collaborator
Author
|
Closed by ActivitySim#950 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR will address #784.
The proposed approach includes some relatively simple "smoke test" of YAML, SPEC, and coefficients config files:
simulate.eval_coefficientsorsimulate.eval_nest_coefficients) is run.As scoped, validating expressions is not included, and the code isn't worried about tables at all.
Currently, there is a small subset of models that get tested, which include simpler configs, one with a preprocessor, and one with nested coefficients. Some basic logging is included. There may be more cases that need to be solved, but I did want to get some feedback / validation before pressing onward. I'll highlight some of my own thoughts below, but also want to get some eyes on this to make sure the contribution meets the issue-brief and will be useful.
Autodiscovery of Model Settings YAML Files
Currently, there is a dict that maps the names of component models to a Pydantic model class, as well as the relevant model file name. This will be pretty cumbersome to maintain.
A better way forward would be identify the default arguments to
model_settings(Pydantic) andmodel_settings_file_name(YAML file) from the signature of each step. I'm not totally sure how to extract this information from the step wrapper around the callable.Error Handling
An important design decision (which is still TBD) will be if any custom errors or warnings are raised by the
settings_checkermodule (currently, this is not included. In my thinking, the main point of the settings checker is to surface any exceptions or warnings that would otherwise be raised at runtime earlier in the run process -- not necessarily to introduce any new error handling. If that is a decent assumption, responsibility for raising problems should be delegated upstream to the underlying Pydantic models and ActivitySim methods.Nested Coefficients
Currently, I am using
simulate.eval_nested_coefficientsto evaluate models with the NESTS property. So far, this seems to behave as I expect -- but I am a little wary that it is misguided. With the settings checker turned off, I have not been been able to identify anything in theprototype_mtcmodel that actually calls this function.Calling the checker
The checker is currently being called from
cli.runfor convenience of development. But I suspect that it needs to be migrated (or added) to theState.runmethod.