-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add a range-validation feature #477
Add a range-validation feature #477
Conversation
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.
Three small changes in line below, after that good to be merged.
year: 2010 | ||
validation: | ||
- range: [ 1, 5 ] | ||
- warning_level: low |
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.
I would remove everything except the range criterion since this is the one we want to test, not upper and lower bound.
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.
Done, and I renamed all test files to have a more intuitive structure.
|
||
|
||
class DataValidationCriteria(IamcDataFilter): | ||
validation: list[DataValidationBounds | DataValidationValue | DataValidationRange] |
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.
I'd refer to the shared base class of the different validation options.
validation: list[DataValidationBounds | DataValidationValue | DataValidationRange] | |
validation: list[DataValidationItem] |
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.
This doesn't seem to work...
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.
As per bilateral discussion, good to be merged from my side.
This PR supersedes #473 because @dc-almeida is on leave and I'd like to use this feature for scenario analysis. The PR implements the review comments by @phackstock made in #473.
Closes #471