-
Notifications
You must be signed in to change notification settings - Fork 166
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
Fix Wb_flows discretization terms for DynamicPipe #3959
Fix Wb_flows discretization terms for DynamicPipe #3959
Conversation
mestinso
commented
Mar 6, 2022
•
edited
Loading
edited
- Fixes made for all structure cases: av_b, a_vb, a_v_b, and av_vb
- The updated discretization terms give more accurate derivatives associated with the volumes
- Energy is now properly conserved (previously av_b, a_vb, and a_v_b had conservation errors)
fe2dcf3
to
f1a8eb6
Compare
- Fixes made for all structure cases: av_b, a_vb, a_v_b, and av_vb - The new discretizations terms give more accurate derivatives associated with the volumes - Energy is now properly conserved (previously av_b, a_vb, and a_v_b had conservation errors)
f1a8eb6
to
59c418e
Compare
@mestinso Thank you for you valuable effort! What I'd like to have are test models (covering all the branches) where the current imlementation raises an assert, but with your fix results in succeeding models. This way we can easily see what the expected purpose of the changes are. Thanks again. |
@beutlich Sure. Do you suggest I add these test models into ModelicaTest as part of this PR? |
Exactly. |
@mestinso did you ever get around to adding those tests to ModelicaTest? |
@bilderbuchi Unfortunately no, but thanks for the prompt here. I expect to get a little time here in the coming weeks, so, hopefully, I'll push an update soon. |
@mestinso Did you got time to add the test to Modelica Test? |
@bilderbuchi @TManikantan @hubertus65 @beutlich Two new tester models have been added. The first tester checks for energy conservation for water flow without the kinetic energy term and the second tester checks for energy conservation for airflow with the kinetic energy term. Note that for the previous version of the DynamicPipe model, 6/12 asserts passed in the first tester and 1/4 asserts passed in the second tester. |
Nice work! It's kinda surprising that in |
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 agree with the comment of @bilderbuchi that the choice of eps = 0 (the default 3rd argument in Modelica.Math.isEqual) is risky and likely toto lead to assertion errors on other platforms, or just with other solvers than those you tested: we have seen such cases before. Therefore I suggest the following: Add a third argument to all calls of isEqual to set eps to a value. My suggestion for eps is in the range of 1e-10 to 1e-12: that is several orders of magnitude larger then the numerical eps, and several orders of magnitude smaller than the solver tolerance.
@bilderbuchi @hubertus65 I went ahead and adjusted those tolerances as recommended. |
Had email with Francesco, and he will review |
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 was also a bit surprised that the tests passed with zero tolerance, but I see those are really round figures (the source is at 3 MJ/kg, and so are the volume enthalpies) so I guess that explains it. I ran Check2 and in that case I see you need indeed a bit of tolerance, around 1 ppm, which makes sense.
I'm currently unable to review the details of the updated model, but given the results of the tests this looks good to me.