Skip to content
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

StateFeature registration via config, service.build, user query #145

Closed
robfitzgerald opened this issue Mar 13, 2024 · 3 comments · Fixed by #186
Closed

StateFeature registration via config, service.build, user query #145

robfitzgerald opened this issue Mar 13, 2024 · 3 comments · Fixed by #186
Assignees

Comments

@robfitzgerald
Copy link
Collaborator

robfitzgerald commented Mar 13, 2024

In order to more clearly delineate between TraversalModels in a hierarchical arrangement, we can enforce that any TraversalModel "registers" the feature it updates.

This would assert the following conventions + code changes:

  1. state features should not be defined at the root of the TOML in a [state] section, but instead, always come from traversal model configuration
  2. each traversal model implements TraversalModel::register_features() (currently state_features()) and returns the state features that it updates
    • if a traversal model depends on a feature from an inner model (hierarchical traversal models), it does not register the feature, but instead, assumes that the features has been registered by the inner model
    • bonus points: the feature names are parameterized in the config JSON used to build a model, allowing us to define the (somewhat arbitrary) feature names at config time
  3. StateModel collects these and builds a state representation as currently implemented
  4. Remove StateModelService
    • we no longer persist StateModel feature configurations
    • we no longer define a top-level [state] in the TOML with globally-set state features
@robfitzgerald
Copy link
Collaborator Author

@nreinicke thinking of re-writing this one, expanding on the idea to a sort of general approach to registering state features with the state model:

  1. state features should not be defined at the root of the TOML in a [state] section, but instead, always come from traversal model configuration
  2. each traversal model implements TraversalModel::state_features() and returns the state features that it updates
    • if a traversal model depends on a feature from an inner model (hierarchical traversal models), it does not register the feature, but instead, assumes that the features has been registered by the inner model
  3. StateModel collects these and builds a state representation

thinking maybe we rename TraversalModel::state_features() to TraversalModel::register_features() to make this clear. along the way, this would set up the DistanceTraversalModel the way this issue currently describes.

what do you think? does this clean things up?

@nreinicke
Copy link
Collaborator

Yeah I like this idea, having the default state features come from the traversal model. Then, at query time, the user could propose a new initial condition for any state feature. If it exists in the state vector that was built by the traversal model it will update it, or, if it doesn't exist, either ignore that parameter or fail.

@robfitzgerald robfitzgerald changed the title load a DistanceTraversalModel to do distance updates TraversalModels register the features that they update Mar 15, 2024
@nreinicke nreinicke self-assigned this Mar 19, 2024
@robfitzgerald
Copy link
Collaborator Author

I think this happens for the most part, but, I don't think user queries can update the state model yet, so I'm keeping it open until we resolve that capability.

@robfitzgerald robfitzgerald changed the title TraversalModels register the features that they update StateFeature registration via config, service.build, user query Apr 5, 2024
@robfitzgerald robfitzgerald self-assigned this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants