Skip to content
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

Props norm #597

Merged
merged 46 commits into from
Apr 9, 2024
Merged

Props norm #597

merged 46 commits into from
Apr 9, 2024

Conversation

SimonRubenDrauz
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 99.00881% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 90.84%. Comparing base (9385283) to head (17735d2).
Report is 20 commits behind head on develop.

❗ Current head 17735d2 differs from pull request most recent head 5b5547c. Consider uploading reports for the commit 5b5547c to get more accurate results

Files Patch % Lines
src/pandapipes/pf/derivative_calculation.py 91.89% 3 Missing ⚠️
.../api/release_cycle/release_control_test_network.py 95.23% 3 Missing ⚠️
src/pandapipes/converter/stanet/preparing_steps.py 50.00% 1 Missing ⚠️
src/pandapipes/test/api/test_convert_format.py 94.11% 1 Missing ⚠️
...st/stanet_comparison/pipeflow_stanet_comparison.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #597      +/-   ##
===========================================
+ Coverage    90.49%   90.84%   +0.35%     
===========================================
  Files          139      139              
  Lines        10549    10996     +447     
===========================================
+ Hits          9546     9989     +443     
- Misses        1003     1007       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dlohmeier dlohmeier left a comment

Choose a reason for hiding this comment

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

Wow, great to see how the differences between the pandapipes and the stanet results vanish here. A few thoughts:

  • How can we ensure that the formulations that we use are correct? I see that you changed a lot of the structure of the derivative calculation, I might have to work through it once more.
  • I think we should also test performance and the number of iterations to convergence in both approaches (v vs. m).
  • Renaming m to mf might help to clarify the meaning of this variable (MFINIT).

Otherwise just a few small comments. I will think about the aspect of calling a property with just a float for another while... don't know if it should be allowed or not.

src/pandapipes/pf/derivative_toolbox.py Outdated Show resolved Hide resolved
src/pandapipes/pf/derivative_toolbox.py Outdated Show resolved Hide resolved
src/pandapipes/pf/derivative_toolbox_numba.py Show resolved Hide resolved
src/pandapipes/pf/derivative_toolbox_numba.py Show resolved Hide resolved
src/pandapipes/pf/pipeflow_setup.py Outdated Show resolved Hide resolved
src/pandapipes/properties/properties_toolbox.py Outdated Show resolved Hide resolved
src/pandapipes/properties/properties_toolbox.py Outdated Show resolved Hide resolved
src/pandapipes/properties/properties_toolbox.py Outdated Show resolved Hide resolved
src/pandapipes/test/stanet_comparison/test_gas_stanet.py Outdated Show resolved Hide resolved
src/pandapipes/pf/result_extraction.py Outdated Show resolved Hide resolved
@dlohmeier
Copy link
Collaborator

Codacy analysis:
grafik

@SimonRubenDrauz
Copy link
Collaborator Author

SimonRubenDrauz commented Mar 4, 2024

  • How can we ensure that the formulations that we use are correct? I see that you changed a lot of the structure of the derivative calculation, I might have to work through it once more.

The best test for me is if the pipeflow is still converging. Additionally, I could check if the pit entries in the first iteration match (taking into account the m and v are connected by density and area). I would say I take the two release tests as reference.

  • I think we should also test performance and the number of iterations to convergence in both approaches (v vs. m).

Two tests were not converging as the number of iterations increased. However, the difference in the end were less than three iterations. But what I could do: For all tests I could save the number of iterations required for both m and v. Afterwards I would just compare both results.

  • Renaming m to mf might help to clarify the meaning of this variable (MFINIT).

What I do not get: What does f stand for. Actually I would say MDOTFINIT, but that might be too long. Maybe MDFINIT?

Otherwise just a few small comments. I will think about the aspect of calling a property with just a float for another while... don't know if it should be allowed or not.

Great! If you come to a solution, let me know.

@dlohmeier
Copy link
Collaborator

I am very happy with MDOTINIT. I just thought that we should have something different than MINIT and came up with MF for mass flow.

Regarding the number of iterations: In many cases, the number of iterations increases if we use bad derivatives. In some cases, the NR might not even converge. Therefore an increased number of iterations is always a hint for errors in the derivative implementation. So I will double check these formulations. And having this information from the tests would be very helpful. Maybe it's even best if we do a test that ensures that a certain number of iterations for a given setup is not exceeded. It should not depend on the working environment. If this number is exceeded, the test fails and we know that we have to address this issue.

@SimonRubenDrauz
Copy link
Collaborator Author

I changed it now from MINIT to MDOTINIT!

@SimonRubenDrauz
Copy link
Collaborator Author

@dlohmeier, did you come up with a solution for calling a property with just a float?

…egative derivation in build_system_matrix.py
- remove unnecessary np.abs
…ing the pipeflow (especially relevant for the bidirectional pipeflow)
# Conflicts:
#	src/pandapipes/component_models/abstract_models/branch_w_internals_models.py
#	src/pandapipes/component_models/pipe_component.py
#	src/pandapipes/idx_branch.py
#	src/pandapipes/pf/build_system_matrix.py
#	src/pandapipes/pf/derivative_calculation.py
@SimonRubenDrauz SimonRubenDrauz merged commit e9c8265 into develop Apr 9, 2024
39 of 40 checks passed
@SimonRubenDrauz SimonRubenDrauz deleted the props_norm branch April 9, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants