-
Notifications
You must be signed in to change notification settings - Fork 54
p3 runtime params reorg #3039
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
p3 runtime params reorg #3039
Conversation
splitting accretion exponents into two specific ones (one for qc and for one qr) will ncessitate a non-bit-for-bit change due to order of operation in finite precision. [NBFB]
due to finitely accurate calculation, incorporating the autoconversion radius parameter at runtime necessitates a not-bit-for-bit change due to finite precision. [NBFB]
|
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 |
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 # 6160 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5916 FAILED (click to see last 100 lines of console output)
|
all DIFFs in #3039 (comment) are expected |
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.
Looks good. Just a minor style comment, not a road blocker.
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 |
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 # 6163 FAILED (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5919 FAILED (click to see last 100 lines of console output)
|
@bartgol I think I'd like this merged sooner rather than later. I promise I will come back to it and clean up these two TODO items due to the BFB stuff. Can you take one more look? Thanks! |
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 |
6c14dc9
to
34cd4f7
Compare
Status Flag 'Pull Request AutoTester' - Error: Jenkins Jobs - A user has pushed a change to the PR before testing completed. NEW EVENT 'committed', ID C_kwDOCEfuetoAKDM0Y2Q0Zjc2ZmVlMjhkMjU1MjYyODRhOTZlMmFkMTlkYjIwMzQxYzU... The Jenkins Jobs will be shutdown; Testing of this PR must occur again. |
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 # 6178 ERROR (click to see last 100 lines of console output)
SCREAM_PullRequest_Autotester_Mappy # 5930 ERROR (click to see last 100 lines of console output)
|
Obv, merging is contingent to testing passing. The nonbfb stuff we saw in p3_tests is not to be overlooked. |
Yes, of course, we are relying on the automerge here (no manual merge). I believe I manually verified all of this on pm-gpu --- the p3_tests were failing due to one of these two NBFB changes. I am fairly certain of that. If the AT returns different results, I will debug over the weekend. |
|
||
Scalar CONS3; | ||
// TODO: always default to second branch after BFB stuff is addressed | ||
if(autoconversion_radius == sp(25.0e-6)) { |
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 is the only part I am so-so about. I think the == was failing without sp
but I cannot remember. It took me a few hours to realize what was happening, but now I can't be so sure what it was (hopefully sp
...)
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.
^^^ @bartgol
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.
Just had a question about formatting.
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
Since AT passed, I am going to merge. I will address Aaron's comment in a separate issue; @hassanbeydoun and I have upcoming P3 PRs, so there will be ample opportunity to address it. |
rename all p3 runtime params, add slightly better docs, reformat code, and encapsulate away the loading of runtime params
[BFB]