power-grid-model icon indicating copy to clipboard operation
power-grid-model copied to clipboard

create generic_branch #729

Open scud-soptim opened this issue 1 year ago • 2 comments

Fixes issue: #729 [FEATURE] Provide a generic_branch

Changes proposed in this PR include:

  • added Component generic_branch as diskussed in #729
  • added testcase generic_branch (self test of the new component)
  • added testcase compare_generic_branch (compare a generic_branch with identical transformer)

scud-soptim avatar Sep 30 '24 15:09 scud-soptim

Hi @sudo-ac,

Thanks for submitting the PR. Before I go in-depth of the content, I would like to give compliment for passing almost all the CI checks for the first try. This rarely happens, even for the active PGM developer.

TonyXiang8787 avatar Oct 01 '24 07:10 TonyXiang8787

Again: thank you for creating the PR. It looks quite good at first sight. I've already started going over this PR, but something else came up. Will continue with the review after that's solved.

mgovers avatar Oct 01 '24 08:10 mgovers

One test failed, but we have no control over it:

************* Module power_grid_model.validation.validation /opt/hostedtoolcache/Python/3.10.15/x64/lib/python3.10/site-packages/power_grid_model/validation/validation.py:1:0: C0302: Too many lines in module (1001/1000) (too-many-lines)

scud-soptim avatar Oct 04 '24 15:10 scud-soptim

One test failed, but we have no control over it:

************* Module power_grid_model.validation.validation /opt/hostedtoolcache/Python/3.10.15/x64/lib/python3.10/site-packages/power_grid_model/validation/validation.py:1:0: C0302: Too many lines in module (1001/1000) (too-many-lines)

Hi @sudo-ac , i'll see if i can come up with a good suggestion on this. The length of the module has always bothered me, but we've never had the time to refactor it. Unfortunately, you're the first one to experience a failing CI because of it. I'll dig into this on Monday.

mgovers avatar Oct 04 '24 16:10 mgovers

@mgovers My proposal is to add a linter exception for this file? To refactor the whole validation function is out of the scope.

Maybe we need to define all the rules in json files. The validation function load the json file and perform certain check.

@sudo-ac I have added a silence line in the beginning of the file

TonyXiang8787 avatar Oct 04 '24 18:10 TonyXiang8787

@TonyXiang8787,

but I added the requested documentation. What was wrong?

scud-soptim avatar Oct 05 '24 08:10 scud-soptim

@TonyXiang8787,

but I added the requested documentation. What was wrong?

Yes, indeed. Sorry I did not pay attention. I just put one more comment.

TonyXiang8787 avatar Oct 05 '24 08:10 TonyXiang8787

Hi @TonyXiang8787,

I added a test case in test_generic_branch, but I will review and update the missing validation on Monday.

scud-soptim avatar Oct 05 '24 09:10 scud-soptim

Hi @TonyXiang8787,

Thank you very much for reviewing this PR. Have a nice weekend!

scud-soptim avatar Oct 05 '24 09:10 scud-soptim

@mgovers My proposal is to add a linter exception for this file? To refactor the whole validation function is out of the scope.

Thank you @TonyXiang8787 for picking that up. I indeed agree that silencing for this PR is the way to go for now. Let's revisit the module after this PR is merged and try to come up with a sustainable solution.

Maybe we need to define all the rules in json files. The validation function load the json file and perform certain check.

I believe the entire validation functionality requires some rework. JSON files sound good, but more complex logic will be difficult to implement that way. Take for example the logic surrounding the default value of the tap position and the one concerning the control side of the transformer tap regulator

mgovers avatar Oct 07 '24 06:10 mgovers