-
Notifications
You must be signed in to change notification settings - Fork 118
Skim name conflicts #939
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
Skim name conflicts #939
Conversation
dhensle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great and the error message output is fantastic. However, this functionality is missing a unit test. I suggest 3 parts to the CI test using omx files with duplicate names: (1) data should load but emit warning if not sharrow, (2) run should crash if using sharrow, (3) run should continue if using sharrow but omx_ignore_patterns is set correctly.
|
@dhensle I added CI testing exactly per your recommendation. Thanks! |
dhensle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks Jeff!
The SkimDataset structure requires every variable to have a unique name. It also merges OMX variables based on time period, so that e.g. BIKETIME__AM and BIKETIME__PM, which would be 2-d arrays in the OMX file, become just two different parts of a 3-d array called BIKETIME in the SkimDataset. This is problematic when the skims also contain a 2-d array called BIKETIME, as that has no temporal dimension, and it gets loaded into a 2-d array in the SkimDataset, with the same name as the 3-d array, and thus one is overwritten and lost.
A workaround was added via the omx_ignore_patterns setting in #867, which allows the user to not load certain skims, but this relies on the model developer or user to set this up correctly.
This PR includes a skims input check to identify this overwriting condition, and raise an error if it is happening, so that the user can correct the condition via (1) the omx_ignore_patterns setting, (2) revising the skim generation process to not create the overlapping named skims in the file in the first place, or (3) renaming one or both skims if the users actually wants both skims variables in the model. The error message generated includes a link to instructions and discussion of these alternatives.
Enhancements to Skim Dataset Handling:
activitysim/core/skim_dataset.py,activitysim/core/skim_dict_factory.py) [1] [2]skim_conflictsattribute in the skim dictionary factory to track and warn about duplicate skim names and conflicts. (activitysim/core/skim_dict_factory.py) [1] [2]activitysim/core/skim_dict_factory.py) [1] [2]Workflow and Dependency Updates:
workflow_dispatchwhile maintaining existing conditions for automatic builds. (.github/workflows/branch-docs.yml)reanddefaultdictimports to support new functionality for skim conflict detection and regex-based ignore rules. (activitysim/core/skim_dataset.py,activitysim/core/skim_dict_factory.py) [1] [2]Documentation Updates:
docs/users-guide/model_anatomy.rst) [1] [2]These changes aim to improve the robustness of skim data handling, provide clearer guidance to users, and ensure compatibility with future updates.