-
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
R_s for each ideal gas record now calculated with R_NASA #4238
R_s for each ideal gas record now calculated with R_NASA #4238
Conversation
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.
Thanks!
Did you change all 1241 manually? Or do you have some automation script?
Might be desirable to have a revison (or info) annotation stating the used value for R.
No I'm not mad, I wrote a quick script! Afterwards I checked a random sample of 5 Happy to add any comments/annotations. |
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 ( |
Having an expression is exactly what I first proposed in #4233 (comment). |
I didn't change all the values of I think this is a reasonable compromise. But, please let me know what changes are required to get this merged. |
See my comments in #4233. I am unsure.
|
As mentioned above (#4238 (comment)), 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:
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.... |
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. |
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.
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? |
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:
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 😄 |
👍 That's correct: in favor of 2. |
Thanks for clarification. |
96e2c6a
to
a4af9fe
Compare
Merging this PR |
Recalculated all the
R_s
values for each Ideal gas record in SingleGasesData, usingR=8.314510
specified in the originalMcBride B.J., Zehe M.J., and Gordon S. (2002) (NASA Glenn coefficients) paper.
See #4233 for discussion.