Skip to content

Conversation

@beutlich
Copy link
Member

@beutlich beutlich commented Jun 9, 2021

Resolves #3666 as proposed by @GaelEnee.

@beutlich beutlich added bug Critical/severe issue L: Media Issue addresses Modelica.Media labels Jun 9, 2021
@beutlich beutlich added this to the MSL4.1.0 milestone Jun 9, 2021
@beutlich beutlich requested review from hubertus65 and thorade June 9, 2021 13:51
@beutlich beutlich requested review from hubertus65 and thorade and removed request for hubertus65 and thorade October 26, 2021 18:55
Copy link
Member

@hubertus65 hubertus65 left a comment

Choose a reason for hiding this comment

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

See my latest comment in the conversation of this PR: I think the derivative is wrong, the correct one is proposed in the original thread, see comments in #3666, and here:
To me, the proposed derivative looks wrong. If the function is:
//pd := xw/(Modelica.Media.Air.ReferenceMoistAir.k_mair + xw)*p;
And for better readability, I propose a shorthand and minor re-write:
Real k = Modelica.Media.Air.ReferenceMoistAir.k_mair; //pd := xw/(k + xw)*p;
I get:
pd_der = (p*xw_der + xw*p_der)/(k + xw) - p*xw*xw_der/(k + xw)^2
Could somebody please verify this, also with the test function from Matthis?

@TManikantan TManikantan assigned hubertus65 and unassigned hubertus65 Feb 15, 2023
@TManikantan
Copy link
Contributor

@hubertus65 @thorade a gentle reminder this issue remains unresolved, Restarting the process of solving it would be really needed.

@TManikantan
Copy link
Contributor

TManikantan commented Apr 5, 2023

@beutlich @wischhusen Did you get around making the changes suggested by @hubertus65 ?

@HansOlsson
Copy link
Contributor

Why do we write such functions at all? Modelica has smoothOrder that automatically derive such a derivative-function from the original, and it would automatically ensure that any other differences disappear - e.g., handling of xws==-1.

Thus my proposal is to just remove the functions pd_pTX_der, h_pTX_der, rho_pTX_der, and u_pTX_der and replace them with smoothOrder=1.

That would avoid such errors, and reduce the maintenance burden.

@beutlich
Copy link
Member Author

By applying smoothOrder=1 and avoiding the explicit derivative functions (see e3c45f2) I get a differentiation error in Dymola 2023

Translation of pd_der_test_smoothOder:

The DAE has 8 scalar unknowns and 8 scalar equations.

Failed to differentiate the equation
var_from_function = Modelica.Media.Air.ReferenceMoistAir.Utilities.pd_pTX(p, T, X);

in order to reduce the DAE index.

Cannot find differentiation function:
Modelica.Media.Air.ReferenceMoistAir.Utilities.pd_pTX(p, T, X)
with respect to time

Failed to reduce the DAE index.

Translation aborted.

ERRORS have been issued.

for the test model:

model pd_der_test_smoothOrder
  replaceable package Medium = Modelica.Media.Air.ReferenceMoistAir;

  Real der_T = 300;
  Real T = 73.15 + der_T * time;
  Real der_p = -1e5;
  Real p = 1e7 + der_p * time;
  Real[Medium.nS] X = Medium.reference_X;

  Real var_from_function = Medium.Utilities.pd_pTX(p, T, X);
  Real timederiv = der(var_from_function);

  annotation (experiment(StopTime=100));
end pd_der_test_smoothOrder;

Is there anything I am missing?

@HansOlsson
Copy link
Contributor

Is there anything I am missing?

Possibly, I'll have to make a branch and test that.

@beutlich
Copy link
Member Author

Is there anything I am missing?

Possibly, I'll have to make a branch and test that.

You can pull my branch: https://github.com/beutlich/ModelicaStandardLibrary/tree/issue3666_avoid-derivative-functions

@TManikantan
Copy link
Contributor

@HansOlsson @beutlich did you get around the solution. Kindly commit the fix if you have arrived at the solution.
Many Thanks

@HansOlsson
Copy link
Contributor

HansOlsson commented Apr 20, 2023

Is there anything I am missing?

Possibly, I'll have to make a branch and test that.

You can pull my branch: https://github.com/beutlich/ModelicaStandardLibrary/tree/issue3666_avoid-derivative-functions

Hm...
TL;DR: Seems we have to delay smoothOrder-change, and do it as another PR.

More detailed:

Dymola currently cannot differentiate this for two reasons:

  • It cannot find derivative of Tlim in Modelica.Media.Air.ReferenceMoistAir.Utilities.pds_pT - note that this derivative isn't needed. It can be worked around by replacing T<=Tlim by a Boolean notAboveTlim; and instead of computing Tlim := ...; compute notAboveTlim:= T<=...;
  • No derivative for Modelica.Media.Air.ReferenceMoistAir.Utilities.f_pT(p, T) (due to limitations for handling functions as arguments to functions)- as far as I can see the current MSL-implementation just assumes this is 1 in the derivative-function! I don't know how correct that is.

I don't know what other tools can do, I could imagine similar limitations - but I think the limitations can be overcome (for functions as argument to functions it depends on the functions). The problematic part for MSL is that the 2nd change is another bug (or simplification) so a correction needs more testing; which is another reason this should be a separate PR.

@TManikantan
Copy link
Contributor

Is there anything I am missing?

Possibly, I'll have to make a branch and test that.

You can pull my branch: https://github.com/beutlich/ModelicaStandardLibrary/tree/issue3666_avoid-derivative-functions

Hm... TL;DR: Seems we have to delay smoothOrder-change, and do it as another PR.

More detailed:

Dymola currently cannot differentiate this for two reasons:

  • It cannot find derivative of Tlim in Modelica.Media.Air.ReferenceMoistAir.Utilities.pds_pT - note that this derivative isn't needed. It can be worked around by replacing T<=Tlim by a Boolean notAboveTlim; and instead of computing Tlim := ...; compute notAboveTlim:= T<=...;
  • No derivative for Modelica.Media.Air.ReferenceMoistAir.Utilities.f_pT(p, T) (due to limitations for handling functions as arguments to functions)- as far as I can see the current MSL-implementation just assumes this is 1 in the derivative-function! I don't know how correct that is.

I don't know what other tools can do, I could imagine similar limitations - but I think the limitations can be overcome (for functions as argument to functions it depends on the functions). The problematic part for MSL is that the 2nd change is another bug (or simplification) so a correction needs more testing; which is another reason this should be a separate PR.

if i am correct ,what i could understand by reading the comment is that you propose to go ahead with the changes suggested by@hubertus65 and then do the simplification work in a different PR. Kindly correct me if i am wrong. Many thanks..

@HansOlsson
Copy link
Contributor

HansOlsson commented Apr 21, 2023

if i am correct ,what i could understand by reading the comment is that you propose to go ahead with the changes suggested by@hubertus65 and then do the simplification work in a different PR. Kindly correct me if i am wrong. Many thanks..

Yes that is a correct understanding.

@TManikantan
Copy link
Contributor

@hubertus65 @thorade Kindly look into the changes done to the fix as per your comments.

@thorade
Copy link
Contributor

thorade commented Apr 24, 2023

I manually derived the expression and arrived at the same result. I also tested with the test model, and the error becomes much smaller, but not zero.

@hubertus65 hubertus65 merged commit 44c8fe5 into modelica:master Apr 25, 2023
@beutlich beutlich deleted the fix-issue3666 branch April 27, 2023 07:57
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

Development

Successfully merging this pull request may close these issues.

ReferenceMoistAir, substances mass conservation issue

6 participants