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

allow query to overwrite state features #186

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

robfitzgerald
Copy link
Collaborator

@robfitzgerald robfitzgerald commented Apr 10, 2024

This PR gives the user the option to overwrite initial values for state features at query time. the user

  • must be overriding an existing state feature by name
  • must match the feature type of the existing (if it was a StateFeature::Distance, it should still be that)
  • can ovewrite the initial value of the feature (go from 0.0 to 10.0 minutes initial time)
  • can also overwrite the enum variant of the unit (go from Kilometers to Meters)

Added issue #185, that would help further protect against invalid overwrites (a percent should still be a percent).

Closes #145.

@nreinicke
Copy link
Collaborator

Yeah I like this feature and think it will be important for things like initial state of charge. When attempting to test this, I'm noticing that the units are getting set appropriately when passing in a custom state feature but I'm not seeing the initial value being updated. For example, if I add the following to our base query in the open street maps example notebook:

        "state_features": {
            "time": {"time_unit": "seconds", "initial": 6000}
        },

The resulting time comes out as 653 seconds:

image

If I don't inject this the time gets reported as 10.89 minutes and so it's using the right units but not the new initial value.

@robfitzgerald
Copy link
Collaborator Author

it's using the right units but not the new initial value

what's weird is the state model does reflect the new initial value in the screenshot above. wondering where this is getting ignored or overwritten. i think i need to check the initial_state function.

@robfitzgerald
Copy link
Collaborator Author

@nreinicke found that StateFeature::get_initial wasn't actually pulling from Distance|Time|Energy StateFeature initial fields, and would divert to CustomFeatureFormat::default() for those cases, for who knows why (i'm looking at you, @robfitzgerald 🙄 )

Copy link
Collaborator

@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find! just pulled the latest changes and it seems to be working as expected, thanks for adding this in!

@robfitzgerald robfitzgerald merged commit 29d944b into main Apr 11, 2024
5 checks passed
@robfitzgerald robfitzgerald deleted the rjf/query-time-state-features branch April 11, 2024 19:10
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 this pull request may close these issues.

StateFeature registration via config, service.build, user query
2 participants