-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of feature 720 asym_line #762
base: main
Are you sure you want to change the base?
Conversation
Hi @leovsch, thanks for the draft PR. I have put the check list in the description of the PR to track where we are. |
Hi @leovsch, Herewith I come back with some suggestions on the implementation of Class memberI suggest having the following two class members for the parameters. The double i_n_;
double base_i_;
ComplexTensor<asymmetric_t> y_series_;
ComplexTensor<asymmetric_t> y_shunt_; ConstructionThe construction of For the construction of if (is_nan(r_na)) {
// if r_na is nan, it means user already does the kron reduction
// we just assign the 3*3 z_series by fill-in with (r + x * imaginary_unit) for each entry
// also fill-in the upper triangle based on symmetry.
}
else {
// if r_na is not nan, the user specifies the neutral conductor parameters, we need to do kron reduction
// in a way that
// z_kl_reduced = z_kl - z_nk * z_nl / z_nn
// k, l = a, b, c
// note: z_nk = z_kn, etc.
// fill-in all the values of 3*3 z_series, also the symmetry
}
y_series_ = inverse(z_series); We do the similar for Calculation parametersSymmetric parameterTo implement y_s = average of diagonal of y_series_;
y_m = average of off-diagonal of y_series_;
y1_series = y_s - y_m You put the Asymmetric parameterTo implement Please learn the function power-grid-model/power_grid_model_c/power_grid_model/include/power_grid_model/component/branch.hpp Lines 205 to 235 in 677c078
|
First implementation version is here some general remarks and questions:
I like working on the project, hope my questions are clear otherwise reach out 👍 |
Hi @leovsch , I will most likely spend some time tomorrow to review this, but at first glance, you've made the right decisions and followed the correct path. Without digging more into it, here's some initial response on your questions.
we try to follow the paradigm "If it looks like a duck, swims like a duck, and quacks like a duck, then it probably is a duck", especially on the things in
we do have something similar for transformers. That functionality resides in
I put it as a topic on the agenda for tomorrow with the rest of the team.
If you like, we can have a call about this. We do have some guidelines, but also a lot of heuristics, so it might be easier to talk to you directly. You can send an email to me ([email protected]) if that is alright with you, and we can schedule a meeting.
That's great to hear! It's very nice to see external contributors working on the project, and we're happy to help you out at any time. Any feedback you have is also highly valued. Again, thank you for your input so far! Martijn |
Thanks again for the contribution! |
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like I said: you're definitely on the right track. a couple ideas.
power_grid_model_c/power_grid_model/include/power_grid_model/common/common.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/common/exception.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/common/three_phase_tensor.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/common/three_phase_tensor.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting can be automatically solved using pre-commit
(see https://power-grid-model.readthedocs.io/en/stable/contribution/CONTRIBUTING.html#pre-commit-hooks ; you can apply the format changes retroactively using pre-commit run --all-files
and run it before committing using pre-commit install
)
} | |
else { | |
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try this once I've completely finished the implementation
return param; | ||
} | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code convention: newline at end of file (EOF)
} | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess this will be fixed if I use the pre-commit?
} | ||
|
||
DoubleComplex average_of_diagonal_of_matrix(const ComplexTensor<asymmetric_t> &matrix) const { | ||
return (matrix(0,0) + matrix(1,1) + matrix(2,2)) / 3.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we already have functionality in three_phase_tensor.hpp
for this. not sure, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked for it but I did not find it, so I left this implementation in for now
} | ||
} | ||
|
||
} // namespace power_grid_model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline at EOF
} // namespace power_grid_model | |
} // namespace power_grid_model | |
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
@leovsch I have compared with OpenDSS and I could not find the different you mentioned. Maybe can you elaborate a bit more about what exactly are we still missing? Then we can make a choice. |
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
Hi Tony, In OpenDSS you can per parameter (R, X, C) decide if you want to supply the full matrix or just the 0th and 1st order values. Example 1: Example 2: And all posible other permutations of the input format. For now I've implemented only the possibility that would support example 1. I can imagine that you would want to generalize such functionality to all possible permutations of the input format just as in OpenDSS. Hope this clarifies it a bit for you. |
@leovsch This is a valid point. Thanks for clarification. We need to discuss this and come back to you later. |
Hi @leovsch, Comeback to your point, we have thoroughly considered different options and decide to go with the attributes you have already defined in this PR. Concretely means: R = always full r matrix (no r1, r0 possible) From practical applications, if the user want to specify a r1/r0, they will almost certain specify x1/x0 instead of full x matrix, verse versa is also true. In that case, the user can just use normal We allow C to be specified in both c0/c1 or full c matrix way because this could be a use-case for the user. |
Thanks for the clarifification. I will leave it as is. |
Hi @leovsch , I see that this branch hasn't been updated with |
power_grid_model_c/power_grid_model/include/power_grid_model/common/matrix_utils.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
I did a rebase of my fork last week monday, I did not have any merge conflicts, does that typically not show up here? Or do you mean that since last monday a lot has changed? |
ohhh that's great. I did not realize that you only did it last monday. Great that you did not have merge conflicts. Usually, the amount of merge conflicts is small for things like a new component, but you never know 😬 |
ComplexTensor<asymmetric_t> _y_series_abc; | ||
ComplexTensor<asymmetric_t> _y_shunt_abc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a suffix, not prefix. So y_series_abc_
else { | ||
ComplexTensor4 r_matrix = ComplexTensor4(asym_line_input.r_aa, asym_line_input.r_bb, asym_line_input.r_cc, asym_line_input.r_nn, asym_line_input.r_ba, asym_line_input.r_ca, asym_line_input.r_na, asym_line_input.r_cb, asym_line_input.r_nb, asym_line_input.r_nc); | ||
ComplexTensor4 x_matrix = ComplexTensor4(asym_line_input.x_aa, asym_line_input.x_bb, asym_line_input.x_cc, asym_line_input.x_nn, asym_line_input.x_ba, asym_line_input.x_ca, asym_line_input.x_na, asym_line_input.x_cb, asym_line_input.x_nb, asym_line_input.x_nc); | ||
ComplexTensor4 y = r_matrix + 1.0i * x_matrix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be called z
, otherwise it is confusing.
DoubleComplex y1_series_ = average_of_diagonal_of_matrix(_y_series_abc) - average_of_off_diagonal_of_matrix(_y_series_abc); | ||
DoubleComplex y1_shunt_ = average_of_diagonal_of_matrix(_y_shunt_abc) - average_of_off_diagonal_of_matrix(_y_shunt_abc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local variables should not have suffix.
member data variables should have suffix.
double const base_i = base_power_1p / (10.0e3 / sqrt3); | ||
double const base_y = base_i * base_i / base_power_1p; | ||
Branch& branch = asym_line; | ||
ComplexTensor<asymmetric_t> const y_series = ComplexTensor<asymmetric_t>(1.87842984-0.42269873i, 1.87842984-0.42269873i, 1.87842984-0.42269873i, -0.62560863-0.00463073i, -0.57187623+0.12931409i, -0.62560863-0.00463073i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do you get those numbers? maybe you want to calculate them based on input
instead of hardcode the number. The future developer can understand the logic easier.
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/component/asym_line.hpp
Outdated
Show resolved
Hide resolved
TEST_CASE("Test asym line") { | ||
|
||
SUBCASE("R and X matrix c0, c1 including neutral") { | ||
AsymLineInput input = {.id = 1, | ||
.from_node = 2, | ||
.to_node = 3, | ||
.from_status = 1, | ||
.to_status = 1, | ||
.r_aa = 0.4369, | ||
.r_ba = 0.0496, | ||
.r_bb = 0.4369, | ||
.r_ca = 0.0485, | ||
.r_cb = 0.0496, | ||
.r_cc = 0.4369, | ||
.r_na = 0.0496, | ||
.r_nb = 0.0485, | ||
.r_nc = 0.0496, | ||
.r_nn = 0.4369, | ||
.x_aa = 0.8538, | ||
.x_ba = 0.7886, | ||
.x_bb = 0.8538, | ||
.x_ca = 0.7663, | ||
.x_cb = 0.7886, | ||
.x_cc = 0.8538, | ||
.x_na = 0.7886, | ||
.x_nb = 0.7663, | ||
.x_nc = 0.7886, | ||
.x_nn = 0.8538, | ||
.c0 = 0.18, | ||
.c1 = 0.308, | ||
.i_n = 216.0}; | ||
|
||
ComplexTensor<asymmetric_t> const y_series = ComplexTensor<asymmetric_t>(1.87842984-0.42269873i, 1.87842984-0.42269873i, 1.87842984-0.42269873i, -0.62560863-0.00463073i, -0.57187623+0.12931409i, -0.62560863-0.00463073i); | ||
execute_subcases(input, y_series); | ||
} | ||
|
||
SUBCASE("R and X matrix, c0, c1 excluding neutral") { | ||
AsymLineInput input = {.id = 2, | ||
.from_node = 2, | ||
.to_node = 3, | ||
.from_status = 1, | ||
.to_status = 1, | ||
.r_aa = 0.4369, | ||
.r_ba = 0.0496, | ||
.r_bb = 0.4369, | ||
.r_ca = 0.0485, | ||
.r_cb = 0.0496, | ||
.r_cc = 0.4369, | ||
.x_aa = 0.8538, | ||
.x_ba = 0.7886, | ||
.x_bb = 0.8538, | ||
.x_ca = 0.7663, | ||
.x_cb = 0.7886, | ||
.x_cc = 0.8538, | ||
.c0 = 0.18, | ||
.c1 = 0.308, | ||
.i_n = 216.0}; | ||
ComplexTensor<asymmetric_t> const y_series = ComplexTensor<asymmetric_t>(1.68079-0.470259i, 1.70433-0.383139i, 1.68079-0.470259i, -0.816117-0.00584238i, -0.769521+0.0817541i, -0.816117-0.00584238i); | ||
execute_subcases(input, y_series); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the test cases similar to test_line.cpp.
However, for the second example the subtestcase Symmetric parameters
both connected fails because of what seems like floating point arithmetic errors. At least if I analyze the values that are being fed to the calc_param_y_sym
function in the code and in the test case I see a slight difference being:
y1_series test:
(2.49699,-0.449976)
y1_shunt test:
(0,96.7611)
y1_series code:
(2.49698,-0.449976)
y1_shunt code:
(0,96.7611)
However, if this is a floating point arithmetic error I would say it is kind of big, so my question is does anyone have any insights here?
Furthermore, the other sub test cases from Symetric results
onwards, seem to pass but again not for both the examples. I copied the test logic and the numbers from test_line.cpp
so I can imagine that something needs to be changed here. But I'm not sure what since these values are not that familiar to me. So could anyone provide me with any insights here?
Finally, the use case I have in mind for this feature only requires support for the following input parameters:
full x matrix
full r matrix
c0, c1 values.
I only have data available that has these parameters which I use in the testcases to test the feature with. Given our earlier discussion we would also like to support te possibility to provide the full c matrix as input. So, my question is could someone provide me with data i.e. a valid R, X and C matrix that I can use in an additional test example?
Hope my questions are clear
Thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason of this deviation is that you hardcoded the translated values into the expected value y_series
with too few significant digits.
I would propose you calculate the y_series
in your test code on-the-fly using the formula and input data. Or (less preferable) you paste the hard-coded value with at least 16 significant digits.
Implement feature:
# 720
Changes proposed in this PR include:
Implementation of feature 720.
Add a new component to support asym_line specification with R, X and C matrix values
Could you please pay extra attention to the points below when reviewing the PR:
I'm a new contributor so pay attention to the coding standards applicable to this project along with achitectural rules etc.
Check list
power-grid-model/power_grid_model_c/power_grid_model/include/power_grid_model/auxiliary/
input.hpp
/update.hpp
/output.hpp
or in the documentation directly)code_generation/data/attribute_classes/
input.json
/update.json
/output.json
+ runcode_generation/code_gen.py
power-grid-model/power_grid_model_c/power_grid_model/include/power_grid_model/component/[component].hpp
file that at least inherits from Base, but in this caseGenericBranch
should inherit fromBranch
power-grid-model/tests/cpp_unit_tests/test_[component].cpp
test_[component].cpp
topower-grid-model/tests/cpp_unit_tests/CMakeLists.txt
power_grid_model_c/power_grid_model/include/power_grid_model/all_components.hpp
main_core/topology.hpp
/input.hpp
/output.hpp
/update.hpp
)code_generation/data/dataset_class_maps/dataset_definitions.json
+ re-runcode_generation/code_gen.py
tests/data
src/power_grid_model/validation/validation.py
+ add corresponding tests