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

Fix algorithm of IdealGases.Common.Functions.s0_Tlow_der #2919

Merged
merged 3 commits into from
Jul 26, 2019

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented May 6, 2019

@beutlich beutlich added bug Critical/severe issue L: Media Issue addresses Modelica.Media labels May 6, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone May 6, 2019
@beutlich beutlich self-assigned this May 6, 2019
Copy link

@finback-at finback-at left a comment

Choose a reason for hiding this comment

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

I checked that the code is same as cp_Tlow(data,T)/T and it's all right.
That you!

@finback-at
Copy link

Sorry, I dropped T_der in my comment. The code value is same as,

T_der * cp_T_low(data,T)/T,

and It's OK!

@tobolar
Copy link
Contributor

tobolar commented May 7, 2019

...I dropped T_der in my comment...

In that case you can simply edit your comment again:
ClipboardX

@beutlich beutlich changed the title Fix algorithm of s0_Tlow_der Fix algorithm of Modelica.Media.IdealGases.Common.Functions.s0_Tlow_der May 7, 2019
@beutlich beutlich changed the title Fix algorithm of Modelica.Media.IdealGases.Common.Functions.s0_Tlow_der Fix algorithm of IdealGases.Common.Functions.s0_Tlow_der May 7, 2019
@thorade
Copy link
Contributor

thorade commented May 7, 2019

My understanding of #1909 was that the smoothOrder annotation is preferred!?
So the better solution would be to use smoothOrder here, and also for cp_T.
The derivative functions still cannot be deleted, because they are called explicitly from MoistAir.

@beutlich
Copy link
Member Author

beutlich commented May 7, 2019

Well, stating the derivative was easier for me (as I do not know which smooth order to set).

@casella
Copy link
Contributor

casella commented May 17, 2019

My understanding of #1909 was that the smoothOrder annotation is preferred!?
So the better solution would be to use smoothOrder here, and also for cp_T.

I agree with @thorade. For functions which are not too involved, I think it is preferrable to leave the tools compute the derivatives via AD. This is certainly the case for these functions, which are based on generalized polynomials, logarithms, and other expressions for which derivative are computed in the most straightforward way by AD.

This makes the codebase smaller and more reliable, and avoids potential errors. AFAIK also OpenModelica supports this option.

@casella
Copy link
Contributor

casella commented May 17, 2019

Well, stating the derivative was easier for me (as I do not know which smooth order to set).

The original functions are analytic, so they can be differentiated as many times as one wants. In practice, thermodynamic systems are at most index 1, so I guess you can write smoothOrder = 2 and that will be more than enough for all practical applications.

Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

I would suggest to add the smoothOrder=2 annotation as explained in my comment.

@beutlich
Copy link
Member Author

I would suggest to add the smoothOrder=2 annotation as explained in my comment.

OK. Resolved.

@beutlich beutlich requested a review from casella May 20, 2019 05:36
@beutlich
Copy link
Member Author

@hubertus65 @wischhusen @casella Please add your review.

@beutlich
Copy link
Member Author

@thorade OK for you to merge?

@thorade
Copy link
Contributor

thorade commented Jul 25, 2019

Yes, good to merge, IIRC this was the version that passed the test.

@beutlich beutlich merged commit e3a3eb6 into modelica:master Jul 26, 2019
@beutlich beutlich deleted the issue2870-fix-s0_Tlow_der branch July 26, 2019 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Critical/severe issue L: Media Issue addresses Modelica.Media
Projects
None yet
5 participants