Add range validation
#473
Add range validation
#473
Conversation
There was a problem hiding this comment.
Thanks for adding this quickly @dc-almeida 👍
A few comments and suggested changes in line.
One more general comment is that I'm not sure we need DataValidationCriteriaRange as a new class. In terms of functionality it's entirely identical to using upper and lower bound, just in a different notation in the yaml file.
Thinking about it, I'm not sure we should even offer two ways to do the same thing. I'd suggest to only support either range or upper_bound and lower_bound. I'd suggest only supporting the more compact range.
On a related note, the classes IamcDataFilter and DataValidationCriteria can and should be combined in my opinion. IamcDataFilter is never used standalone and only DataValidationCriteria inherits from it. That's probably better addressed though when tackling #469.
| class DataValidationCriteriaMultiple(IamcDataFilter): | ||
| validation: ( | ||
| list[DataValidationCriteriaValue | DataValidationCriteriaBounds] | None | ||
| list[ | ||
| DataValidationCriteriaValue |
There was a problem hiding this comment.
I would change this to:
| DataValidationCriteriaValue | |
| DataValidationCriteria |
| list[DataValidationCriteriaValue | DataValidationCriteriaBounds] | None | ||
| list[ | ||
| DataValidationCriteriaValue | ||
| | DataValidationCriteriaBounds |
There was a problem hiding this comment.
| | DataValidationCriteriaBounds |
| list[ | ||
| DataValidationCriteriaValue | ||
| | DataValidationCriteriaBounds | ||
| | DataValidationCriteriaRange |
There was a problem hiding this comment.
| | DataValidationCriteriaRange |
| range: list[float] = Field(..., min_length=2, max_length=2) | ||
|
|
||
| @model_validator(mode="after") | ||
| def check_range_is_valid(self): |
There was a problem hiding this comment.
This validator represents a design choice in my opinion. We for sure want the range to be ordered and the way I see it, is we have two options to ensure this:
- (implemented here) we check and throw and error if the range is not ordered
- We take the input value for
range, sort it and only then save it into the attribute
The same design question presents itself when talking about the ordering of the severity levels. Currently we through an error if they are not sorted by the most severe first. Alternatively, we could also take the input, sort it and then save it.
The question does not (and probably should not) be answered, I just wanted to raise the point to be discussed at some stage. My (slight) personal preference would be to take whatever the user provides us, sort it and then work with it. This way we could potentially reduce the amount of frustrated nomenclature users.
There was a problem hiding this comment.
The design-choice on checking the order of severity levels was my suggestion - I think it's better to force users to follow an easy-to-read convention. And it also avoids the ordering as one extra step in the code, improving readability.
There was a problem hiding this comment.
The design-choice on checking the order of severity levels was my suggestion - I think it's better to force users to follow an easy-to-read convention. And it also avoids the ordering as one extra step in the code, improving readability.
Fair point in making that design choice and sticking to it here as well, works for me. You don't really save any code though since in one case you'd be sorting the inputs, in the other case you're checking that they arrive sorted, six and one half dozen.
| @model_validator(mode="after") | ||
| def check_range_is_valid(self): | ||
| if self.range[0] > self.range[1]: | ||
| raise ValueError("Validation range is invalid: " + str(self.criteria)) |
There was a problem hiding this comment.
This error message does not contain enough information for the user to fix the issue:
| raise ValueError("Validation range is invalid: " + str(self.criteria)) | |
| raise ValueError("Validation range must be ordered with the smaller value first: " + str(self.criteria)) |
| raise ValueError("Validation range is invalid: " + str(self.criteria)) | ||
| return self | ||
|
|
||
| @computed_field |
There was a problem hiding this comment.
Nice use of the computed_field decorator 👍 To make things more explicit and follow the pydantic docs (https://docs.pydantic.dev/2.10/concepts/fields/#the-computed_field-decorator), I'd adding the property decorator as well for this and lower_bound.
Disagree, there are actually two distinct use cases: have both upper and lower bounds (i.e., a range), vs. having only one of those. |
Fair point, still, having upper and lower bound entirely covers the use of range. If you want to provide users the opportunity to use both option then I think having two classes is the cleanest way to implement that. |
#471
Adds a
rangeattribute to set lower/upper bounds in validation criteria.Adds a simple test showing it works equivalently to bounds.
Missing test(s) to raise validation error.