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

feat: extend DBFlexContextSchema validation to only allow valid units #1364

Merged
merged 27 commits into from
Mar 18, 2025

Conversation

joshuaunity
Copy link
Contributor

@joshuaunity joshuaunity commented Mar 11, 2025

Description

This PR extends the FlexContextSchema validation to also cover for field value unit validations

Look & Feel

No visual description as it's a backend

How to test

  1. Go to asset's page and click on an asset
  2. Go ahead to edit the asset's flex-context
  3. Try any of the following
  • Add a non-energy price unit to any price field
  • Add a non-power unit to any power field

Further Improvements

In this PR I noticed a rarely occurring bug with the modal UI, a follow up PR will be made to fix this bug and another one reported by @nhoening

Related Items

This PR closes #1353


  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

@joshuaunity joshuaunity self-assigned this Mar 11, 2025
@joshuaunity joshuaunity requested a review from nhoening March 11, 2025 11:47
@joshuaunity
Copy link
Contributor Author

@nhoening @Flix6x

I followed the discussion on the issue ticket, but at the en,d it wasn't clear where we decided to put this validation, the FlexContextSchema or the DBFlexContextSchema

@Flix6x
Copy link
Contributor

Flix6x commented Mar 11, 2025

DBFlexContextSchema

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Thanks, this works well, I have a comment on making the code more explicit.

@joshuaunity joshuaunity requested review from nhoening and Flix6x March 12, 2025 21:14
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Thanks, I'm happy!

(aside from one small docstring clarification about future work)

Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Thank you for adding a lot of test cases! This made it easier to spot that we are not yet distinguishing correctly between energy prices and capacity prices.

@joshuaunity joshuaunity requested a review from Flix6x March 13, 2025 12:38
@joshuaunity joshuaunity requested review from Flix6x and removed request for Flix6x March 14, 2025 10:56
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Thanks, the new function works well. Thanks for adding the doctests, too. I only have a few more requests mostly regarding serializing the field names in error messages of the price fields.

@joshuaunity joshuaunity requested a review from Flix6x March 17, 2025 08:16
@joshuaunity joshuaunity requested a review from Flix6x March 17, 2025 10:55
Signed-off-by: Joshua Edward <[email protected]>
@joshuaunity joshuaunity requested a review from Flix6x March 17, 2025 15:09
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Job well done! Again, the addition of the various test cases really helped us to prevent introducing bugs, thank you for adding those. Units are tricky, but really well suited for unit tests (what's in a name).

Please note that I removed a validator that I suspected to be redundant. Specifically, passing a time series specification (as a list of dicts) to the DBFlexContextSchema already lead to a ValidationError, as you can see here, where the same test case (also) triggered an alternative message. I actually touched up that alternative message in e48a763 and 6065a5a.

I also made several corrections to distinguish energy prices from capacity prices.

@Flix6x Flix6x merged commit 1b8906f into main Mar 18, 2025
10 checks passed
@Flix6x Flix6x deleted the feat/flexcontext-extended-validation branch March 18, 2025 19:19
@Flix6x Flix6x changed the title feat: extend flexContext schema validation to only allow valid units feat: extend DBFlexContextSchema validation to only allow valid units Mar 18, 2025
@nhoening nhoening added this to the 0.25.0 milestone Mar 18, 2025
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.

Validate unit types in flex context schema
3 participants