-
Notifications
You must be signed in to change notification settings - Fork 6
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
Change default constructor of Pack to init to 0 #318
Conversation
While invalid seems nice, it plays badly with Kokkos scan operations, where the accumulation var is created using the value type default constructor.
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
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: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Mappy # 467 (click to expand)
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Weaver # 558 (click to expand)
|
The minmax of a view must not use the padding at the end
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
NOTICE: The AutoTester has encountered an internal error (usually a Communications Timeout), testing will be restarted, previous tests may still be running but will be ignored by the AutoTester... |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
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: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Mappy # 469 (click to expand)
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Weaver # 560 (click to expand)
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
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: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Mappy # 470 (click to expand)
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Weaver # 561 (click to expand)
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
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: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Mappy # 471 (click to expand)
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Weaver # 562 (click to expand)
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
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: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Mappy # 474 (click to expand)
Console Output (last 100 lines) : EKAT_PullRequest_Autotester_Weaver # 564 (click to expand)
|
@jgfouca I killed the weaver job cause it was doing the cleanup action, which, due to the FS issues, was taking forever. But the build passed, as you can see from the log. If you approve, go ahead and merge using the force. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: EKAT_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: EKAT_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bartgol |
NOTICE: The AutoTester has encountered an internal error (usually a Communications Timeout), testing will be restarted, previous tests may still be running but will be ignored by the AutoTester... |
Status Flag 'Pull Request AutoTester' - Failure: Timed out waiting for job EKAT_PullRequest_Autotester_Weaver to start: Total Wait = 1803
|
Motivation
While leaving entries to be invalid seems like a nice idea (allows to track uninited values better), it plays badly with Kokkos parallel_scan, where the accumulation variable is initialized as
ValueType accum = ValueType();
, which assumes that the value initialization inits to 0 (or, to be precise, to the identity value for the sum operation). It would be nice if Kokkos didbut this may break existing code (e.g., code that uses some
ValueType
for which there is no specialization ofreduction_identity
). I still think they could use some SFINAE magic to usereduction_identity
if the right specialization exists, and resort toValueType()
otherwise. Alas, I am not a kokkos dev.So the solution is to make our default constructor init to 0.
I threw in also the change of a few ctor, to call the corresponding
set
method, to avoid duplication of code.E3SM Stakeholder Feedback
The PR in E3SM that updates to Kokkos 4.2 has exposed this issue. In the version of Kokkos in current master, the accum var is inited as
value_type accum = 0
, which works correctly. But updating to Kokkos 4.2 will require this change.Testing
I tested EAMxx with the Kokkos 4.2 branch on chrysalis. Without this change, I get NaN's from scan operation, while with this change I get correct numbers.