-
Notifications
You must be signed in to change notification settings - Fork 54
add runtime options for p3 #3033
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
Conversation
|
Let's fix the formatting, add some inline docs, and then confirm this is all BFB. We can merge it afterwards. I will push a formatting commit soon. |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
<p3_autoconversion_prefactor type="real" doc="P3 autoconversion_prefactor (scale factor in autoconversion)">1350.0</p3_autoconversion_prefactor> | ||
<p3_autoconversion_qc_exp type="real" doc="P3 autoconversion qc exponent">2.47</p3_autoconversion_qc_exp> | ||
<p3_autoconversion_nc_exp type="real" doc="P3 autoconversion nc exponent">1.79</p3_autoconversion_nc_exp> | ||
<p3_autoconversion_radius type="real" doc="P3 autoconversion radius of new rain drops">0.000025</p3_autoconversion_radius> |
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.
It may be a good idea to add the units in the doc string.
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 wanna edit the docs for all p3 constants tbh...
{ | ||
public: | ||
Scalar p3_autoconversion_prefactor = 1350.0; | ||
Scalar p3_autoconversion_qc_exp = 2.47; |
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.
Random question: do we know why the struct P3_Constants
is in physics/share
rathe than physics/p3
? I don't see it used anywhere else. Do we foresee other parts of eamxx to interact with it?
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, I think we should pull it out of physics/share
. I fear that in the future we may see physics/share/physics_constants.hpp
as a dumping ground for a bunch of different processes that may or may not be in use.
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.
This was done in #2655. I agree with both of you on not having these here, but I view that as a separate PR
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 Naser for volunteering to do it an a separate PR... :P
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.
If you look inside components/eamxx/src/physics/share/physics_constants.hpp, I think it is already the case that it is a dumping ground ...
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.
(jk)
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 can, but it will have to wait a bit :D
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
SCREAM_PullRequest_Autotester_Weaver # 6131 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5893 FAILED (click to see last 100 lines of console output)
|
nc2nr_autoconv_tend.set(qc_not_small, qc2qr_autoconv_tend*nc_incld/qc_incld); | ||
ncautr.set( | ||
qc_not_small, | ||
qc2qr_autoconv_tend / (CONS2 * pow(p3_autoconversion_radius, 3))); |
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.
@AaronDonahue @bartgol the DIFF is likely due to this:
CONS3 = 1./(CONS2*1.562500000000000e-14)
CONS3 = 1./(CONS2*(25e-6)**3)
(25e-6)**3 != 1.562500000000000e-14
(floating precision)
What I can do to keep BFB is to do an ugly if-else such that if the radius is 25e-6, we use CONS3 = 1./(CONS2*1.562500000000000e-14)
and if not, we calculate it with the pow
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.
Could you put the if-else in and test for BFB, then maybe you could report that you've done that and the results and we can then switch to non-BFB knowing it's at the precision level that it is diffing?
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.
will do
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.
Tried, still getting a DIFF. Let's see what the AT returns...
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 don't see "maintain BFB with old code" as a priority. And I really don't like the if-else. It's confusing at best. Since this is clearly a rounding error coming from storing a non-fp-representable number, this non-bfbness is completely beningn. We should NOT alter the code just to be bfb with what was before. What was before is not the ultimate truth.
I vote for blessing the diff, and keep the more humane implementation...
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.
Yeah, I agree. I still think we are not getting BFB with it anyway... I will try to redo this PR carefully from scratch to avoid any weird behavior
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 not keeping the else-if and blessing the diff. Just want to make sure we understand the diff is a precision issue and not something else.
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.
It's something else. I am going to close this PR and reset the branch I took away from Hassan. I will revisit the whole thing again soon-ish.
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Using Repos:
Pull Request Author: mahf708 |
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.
Imho, we should revert/remove the commit to get bfb-ness. It's a beningn diff.
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
SCREAM_PullRequest_Autotester_Weaver # 6132 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5894 FAILED (click to see last 100 lines of console output)
|
add options for p3 runtime
[bfb]