-
Notifications
You must be signed in to change notification settings - Fork 62
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
calling 'calculate_derivatives_thermal' once #563
calling 'calculate_derivatives_thermal' once #563
Conversation
SimonRubenDrauz
commented
Sep 8, 2023
•
edited
Loading
edited
- Just calling 'calculate_derivatives_thermal' once instead of calling it for each component seperately
- renaming TINIT and T_OUT in TINIT and TOUTINIT in branch
- rename TINIT in TINIT_IN and T_OUT in TINIT_OUT in Branch
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #563 +/- ##
===========================================
- Coverage 85.23% 85.06% -0.17%
===========================================
Files 90 90
Lines 6202 6181 -21
===========================================
- Hits 5286 5258 -28
- Misses 916 923 +7
☔ View full report in Codecov by Sentry. |
pandapipes/component_models/abstract_models/branch_w_internals_models.py
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.
In general, I like this whole approach and it's about time to put together all branches when calculating the thermal derivatives. One thing that strikes me is the variable TINIT / TINIT_IN that we define in branch_idx and that you rename here. This is not a solver variable, so I would suggest to throw out this variable fully. It leads to confusion, because "the branch temperature" is not defined. It would make more sense to me to always define which temperatures precisely I need in what constellation. If I want the intermediate of the two neighboring junctions, I have to get from_nodes and to_nodes beforehand.
@dlohmeier: What I do not understand: What do you mean by throwing out the variable? Shall we rename it? I wouldn't throw it out completely because we use it in different functions. |
Yes, it is used, but it can be replaced by a call like
The advantage is that there is more control over what value really is needed in the specific case. If we assume that a fluid property of a branch element depends on the branch temperature, it makes sense to rethink how the temperature of this branch should be calculated. In case of thermal calculation, we would definitely need to change the above statement to:
but in case of hydraulic simulation, T_OUT might not be properly initialized. We have to check this again, but I think that it is more useful to assume that the temperature is a very local (point) variable that cannot be a branch variable and each time you need "the temperature" of a branch, you have to define it yourself. |
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.
This now looks good to me.
@dlohmeier: I just checked how T_OUT is initialized in a pipe. Actually it is exactly what you described above: Thus, both equations above should get the same result in the hyadraulics calculation. So we could use this definition: t_init = (node_pit[from_nodes, T_INIT] + branch_pit[:, T_OUT]) / 2 to always calculate the correct temperature: |
I reconsidered your point and decided to throw out TINIT completely. I agree that it is really misleading and causes confusion. And the result is that I found something what really confuses me. |
@@ -95,7 +95,7 @@ def test_heat_only(use_numba): | |||
nonlinear_method="automatic", mode="hydraulics", use_numba=use_numba) | |||
|
|||
p = ntw._pit["node"][:, 5] | |||
v = ntw._pit["branch"][:, 12] |
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.
Wouldn't it make more sense to import this? Less error prone...
I like the adjustments here. Guess we have to look at the pump again, but it seems that you were able to fix a few error prone implementations here. |
@dlohmeier, can we merge this PR. All changes have been realized which you recommended. |
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.
Looks really good to me. This should improve performance and be a great preparation of using numba for thermal simulation as well, and we got rid of yet another branch variable. :-D