PowerGridModel/power-grid-model

[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 attribute k.
  • 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

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