search

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
453 stars 165 forks source link

R_s for each ideal gas record now calculated with R_NASA #4238

Closed Parti-Gyle closed 6 months ago

Parti-Gyle commented 7 months ago

Recalculated all the R_s values for each Ideal gas record in SingleGasesData, using R=8.314510 specified in the original McBride B.J., Zehe M.J., and Gordon S. (2002) (NASA Glenn coefficients) paper.

See #4233 for discussion.

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

Parti-Gyle commented 7 months ago

No I'm not mad, I wrote a quick script! Afterwards I checked a random sample of 5 R_s values by hand and they checked out, as well as checking there were 1241 changes (same number as the number of records).

Happy to add any comments/annotations. Maybe a library maintainer can advice on how they'd like the change documented?

mestinso commented 7 months ago

It seems to me that it would have been better if, instead of recalculating and entering each R value one by one, you just replaced the R values with an expression in its place (R_NASA2002/MM). This way, the R value is more directly traceable to the original source and someone (like yourself) doesn't have to reverse engineer what the universal R value must have been in order to catch a bug.

beutlich commented 7 months ago

It seems to me that it would have been better if, instead of recalculating and entering each R value one by one, you just replaced the R values with an expression in its place (R_NASA2002/MM).

Having an expression is exactly what I first proposed in https://github.com/modelica/ModelicaStandardLibrary/issues/4233#issuecomment-1819417794.

Parti-Gyle commented 7 months ago

I didn't change all the values of R_s to an expression, as I didn't think having an old value for R that any user could inadvertently reference was a good idea. There should only be one value for the constant R, stored in the constants package. As suggested #4233, these ideal gas routines should ideally use the now fixed ideal gas constant. But, this would require re-calculating all the polynomial coefficients for every species, which I'm not prepared to do.

I think this is a reasonable compromise. But, please let me know what changes are required to get this merged. Thanks

mestinso commented 7 months ago

Agreed with your concern, but this is why the constant should live within the relevant subpackage and should generally not be used outside that subpackage. Note that this isn't the first spot in MSL where different universal R values are being used instead of the one in Modelica.Constants. See an 8.314371 R value here: Modelica.Media.Air.ReferenceMoistAir.Utilities.Water95_Utilities or the same 8.314510 R value here: Modelica.Media.Air.ReferenceAir.Air_Utilities.Basic.

So, for example, something like following could be done: image It should be clear from the constant name and its location that this constant is intended for local use.

One annoying thing about this implementation is that while it's much more traceable where the R_s value came from, it's a little more annoying to make a script to automatically make the change since the expression has the instance name in it.

hubertus65 commented 6 months ago

See my comments in #4233. I am unsure.

mestinso commented 6 months ago

See my comments in #4233. I am unsure.

* If we add more advanced mixture routines later, I think the advice from NIST to only use the latest R for everything is sound.

* That would require reverting the proposed change. Often, the NASA data is the only data available. for some fluids.

* My first thought was yes, the "local R 2002" would be the right choice, but not after reading the discussion at NIST.

As mentioned above (https://github.com/modelica/ModelicaStandardLibrary/pull/4238#issuecomment-1861789063), I would point out that this isn't the only spot in MSL where "local R" values are used. If we are proposing using the new final R value here, we should probably use the new final R value everywhere. Why be inconsistent? Arguably that is a bigger decision beyond this PR (which just fixes a bug with the original NASA gas implementation).

With that said, I think it's worth pointing out that there are some key differences and considerations in MSL compared to REFPROP that may lead to different decisions from what NIST decided: 1) As far as I know, we aren't facing any current pain points due to different gas constants being used. Whereas they cite live issues related to "enhanced mixture functions" among other things. For us, any pain points are hypothetical ones for functionalities not yet implemented. 2) NIST still maintains a flag that the user can turn on to go back to the original R value. This is important for a) routine verification, b) if max accuracy is required (rare), or c) if the user wants to avoid any sort of change across REFPROP releases. In our case, I don't think we are going to support any sort of flag (new gas constant vs original gas constant), so if we switch the gas constant now we have introduced differences from the primary reference with no easy way to revert on demand. This means that a user or developer cannot easily verify whether the routine is implemented correctly. There will be (minor) differences and one cannot be sure if those are differences due to the R value difference, some bug, or both. REFPROP doesn't have this issue because they can fall back to the original routine on demand for verification. 3) Lastly I would comment that Eric Lemmon expressed hesitation (ie it's not a black-and-white decision) and they didn't even fully make the switch (unless I'm misinterpreting his comment): I think he mentioned oxygen, water, nitrogen, argon, and others that still use the original R value.

Given these facts and considerations, I don't see much upside in MSL to switch over to the new R value across the board, whereas I do see various downsides. With that said, as new routines are released which displace the old ones, I think it would be a great goal to adopt them and eventually reach a future state with routines that only use the new R value. Maybe an MSL 5.0 goal....

hubertus65 commented 6 months ago

Ok,Matt, @mestinso, I buy your arguments and am fine with the proposed change. Thanks for your efforts! @GallLeo, please be aware that this change might lead to regression test failures for any test involving ideal gases, depending on the tolerance chosen. THEY CAN BE IGNORED, if sufficiently small.

beutlich commented 6 months ago

After some through thinking about this case, I agree with the proposed changes.

Just to be on the safe side: Which means, the PR needs to be updated first to not use/introduce R_NASA, right?

casella commented 6 months ago

Thorny issue indeed, when constants change all the time. BTW, this also happens with other constants, such as the critical pressure of water, which changed several times during my career. The discussion split between this PR and #4233 makes things easier 😅.

As @beutlich, I am also a bit confused about exactly what is the final decision. I'll try to summarize what I understood of the whole discussion:

So, I understand we now have three choices:

  1. Leave SingleGasesData as it is. No regressions, but it's nether using the ultimate value of R, nor the original one of the Glenn paper
  2. Merge this PR to make SingleGasesData consistent with the Glenn paper, tough it uses a "local" value of R, as other fluid models in MSL do. Very small regressions possible
  3. Modify the PR so that it uses the ultimate value or R = Modelica.Constants.R instead of the NASA Glenn value. Even smaller regressions possible.

I understand @mestinso and @hubertus65 are both in favour of 2., as I also am, for what it's worth. Please confirm before we proceed.

Then, try to get ChatGPT-4 to figure this out automatically from all the posts. Just kidding 😄

mestinso commented 6 months ago

I understand @mestinso and @hubertus65 are both in favour of 2., as I also am, for what it's worth. Please confirm before we proceed.

:+1: That's correct: in favor of 2.

beutlich commented 6 months ago

I understand @mestinso and @hubertus65 are both in favour of 2., as I also am, for what it's worth. Please confirm before we proceed.

Thanks for clarification.

Harisankar-Allimangalath commented 6 months ago

Merging this PR