Skip to content

Conversation

@kornerc
Copy link
Contributor

@kornerc kornerc commented Dec 10, 2025

Fixes issue: #1204

Signed-off-by: Clemens Korner <clemens.korner@gmail.com>
@kornerc kornerc force-pushed the transformer-3w-loading branch from 2ce525e to bc5210b Compare December 10, 2025 19:46
@mgovers mgovers added the feature New feature or request label Dec 11, 2025
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kornerc,

Great start! Already had a quick look and found 2 minor things. Can you please also update the documentation?

Comment on lines 110 to 112
double loading(double s_1, double s_2, double s_3) const final {
return std::max({s_1 / sn_1_, s_2 / sn_2_, s_3 / sn_3_});
return std::max({loading_1(s_1), loading_2(s_2), loading_3(s_3)});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may even move this implementation to the Branch3 class as it is probably generic to say that the loading is the max loading across all sides for any possible Branch3 combined. Optional, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can do that.
Should this method also be final?
I guess to be on the safe, it would be useful that if needed subclasses can overrite this method.

Copy link
Member

@mgovers mgovers Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you don't need to make it final because it implies virtual and it means that it is probably not inlined. without virtual is better because inline is implied (as it's a member function in a header file). if in the future (quite unlikely but who knows) we do need to make it virtual, we always can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of loading is now in Branch3.

Co-authored-by: Martijn Govers <martygovers@hotmail.com>
Signed-off-by: Clemens Korner <clemens.korner@gmail.com>
@kornerc
Copy link
Contributor Author

kornerc commented Dec 11, 2025

Three other points which I have noticed:

  • In order to run code_gen.py, the libraries jinja2 and dataclasses_json are needed which are currently not in pyproject.toml.
    Since Enable UV in build and CI #1206 made major changes in pyproject.toml, I can create afterwards a separate PR which adds a new dependency group codegen which contains these two dependencies.
  • Do I need to update the test data files in tests/data/power_flow/three-winding-transformer?
    If yes what is the recommended way to do so?
  • The templates for the auto generated files (as well as the auto generated files themself) e.g.
    // This file is automatically generated. DO NOT modify it manually!

    state in the header that they should not be modified since they were automatically generated.
    Would it make sense to also mention the source from which file they were autogenerated the simppify the backtracing?
    E.g.
    // This file is automatically generated. DO NOT modify it manually!
    // Sources: code_generation/code_gen.py, dataset_class_maps.cpp.jinja
    

@mgovers
Copy link
Member

mgovers commented Dec 11, 2025

In order to run code_gen.py, the libraries jinja2 and dataclasses_json are needed which are currently not in pyproject.toml.
Since #1206 made major changes in pyproject.toml, I can create afterwards a separate PR which adds a new dependency group codegen which contains these two dependencies.

There is a separate requirements.txt in the code_gen/ subdirectory. The reasoning is that it is rare to use it (we don't make API changes that often) and that a virtual environment should - on average - be clean. That said, I agree that that is legacy from the old days and it's good to update it to the modern way of doing things, so please do make a PR.

Do I need to update the test data files in tests/data/power_flow/three-winding-transformer?
If yes what is the recommended way to do so?

In principle, all existing tests will remain functioning as-is. It would be nice to update one or two of the cases to reflect the new additions or add a separate one, but you don't have to revisit all of them. To update them, you can open one of the sym_output.json and/or asym_output.json files that contain a 3-winding transformer, and then add the loading_(1|2|3) attributes to the JSON with the correct values. You can do so by manually calculating the values from your input and the per-side power output.

The templates for the auto generated files (as well as the auto generated files themself) e.g.

power-grid-model/code_generation/templates/power_grid_model_c/power_grid_model_c/src/dataset_class_maps.cpp.jinja

Line 5 in f8777be
// This file is automatically generated. DO NOT modify it manually!

state in the header that they should not be modified since they were automatically generated.
Would it make sense to also mention the source from which file they were autogenerated the simppify the backtracing?
E.g.

// This file is automatically generated. DO NOT modify it manually!
// Sources: code_generation/code_gen.py, dataset_class_maps.cpp.jinja

I think it would be nice to have that, but please do so in a separate PR (if you like, you can make a GitHub issue for it). Similarly, the templates are - of course - technically not automatically generated, so maybe you can put that string in the code_gen.py and "load" it into the template files while generating output by something along the lines of {{ generated_file_message }}

@figueroa1395 figueroa1395 changed the title Implement loading_<site> for three winding transformer Implement loading_<side> for three winding transformer Dec 12, 2025
@figueroa1395
Copy link
Member

@kornerc small remark. I've assigned this to you and made a small change in the title: site -> side

Signed-off-by: Clemens Korner <clemens.korner@gmail.com>
@kornerc kornerc marked this pull request as ready for review December 12, 2025 15:12
@kornerc
Copy link
Contributor Author

kornerc commented Dec 12, 2025

The PR is ready to be merged from my perspective!

@kornerc small remark. I've assigned this to you and made a small change in the title: site -> side

Thanks for fixing!

In principle, all existing tests will remain functioning as-is. It would be nice to update one or two of the cases to reflect the new additions or add a separate one, but you don't have to revisit all of them. To update them, you can open one of the sym_output.json and/or asym_output.json files that contain a 3-winding transformer, and then add the loading_(1|2|3) attributes to the JSON with the correct values. You can do so by manually calculating the values from your input and the per-side power output.

I've did that.
Only one minor remark: I use the devcontatiner as it is provided by this repo.
After editing asym_output_batch.json in VS Code and saving the file, it is automatically reformated with the devcontainer settings, thus, some minor formatting changes happened in this file too.

There is a separate requirements.txt in the code_gen/ subdirectory. The reasoning is that it is rare to use it (we don't make API changes that often) and that a virtual environment should - on average - be clean. That said, I agree that that is legacy from the old days and it's good to update it to the modern way of doing things, so please do make a PR.

I think it would be nice to have that, but please do so in a separate PR (if you like, you can make a GitHub issue for it). Similarly, the templates are - of course - technically not automatically generated, so maybe you can put that string in the code_gen.py and "load" it into the template files while generating output by something along the lines of {{ generated_file_message }}

I agree, let's don't address this in this PR.
I will open separate issues and make the implementation for these issues in separate PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants