[BUG] missing validation of k for generic_branch
Closed this issue · 6 comments
Describe the bug
The parameter k (ratio of the generic branch) should be validated. Zero values lead to exception errors during calculation.
To Reproduce
set k of a generic_branch to zero
Expected behavior
Validation check should detect wrong value k
Solution
Add check greater than zero in validation.py
def validate_generic_branch(data: SingleDataset) -> list[ValidationError]:
errors = validate_branch(data, ComponentType.generic_branch)
errors += all_greater_than_zero(data, ComponentType.generic_branch, "k")
return errors
Hi @sudo-ac , thank you for finding and reporting this. Is this something you would like to implement?
A couple considerations:
- The documentation needs to be updated in https://power-grid-model.readthedocs.io/en/stable/user_manual/components.html#id9 . Specifically:
- the
valid values
column should be> 0
for attributek
.
- the
- The data validator needs to be updated:
- the check mentioned in the description has to be added to
src\power_grid_model\validation\validation.py
- appropriate unit tests need to be created in
tests\unit\validation
- the check mentioned in the description has to be added to
If possible, we would like to add this this week, so that it can be included in the next version bump. Everything is ready for the version bump except for the final parts of documentation for columnar data, so it's kind of high priority.
If you don't have time for it before that, that's fine, but then we will have top pick it up ourselves, or we'll need to explicitly scope it out of the version (c.c. @petersalemink95 )
About the suggestion by @mgovers to add a check to the constructor of GenericBranch
: please don't. We don't check for valid values in the core. That's undefined behavior. That's why we have the input/update validation
It is not necessary to be done before the version bump, since it's not a bug in the core itself, but a missing input validation check. Nevertheless, it's of course nice if this is fixed soon :)
I would only correct the validation function as I suggested. In the case where k = 0.0, the following message would appear:
power_grid_model.validation.assertions.ValidationException:
There is a validation error in input_data:
Field 'k' is not greater than zero for 1 ComponentType.generic_branch.
The constructor of generic_branch
sets k to 1.0 if it is not provided. I would also adapt the documentation as suggested. Yes, and I would like to implement this.
Great! cfr. quick discussion with @petersalemink95 , I removed the part on the core implementation from my earlier comment