SHOC related diagnostic output #2975
Conversation
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bogensch |
|
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_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5793 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 6019 PASSED (click to see last 100 lines of console output) |
|
|
I approved because the guard on the additions is satisfactory. I don't actually know if the additions make sense or not. I haven't reviewed that. |
| @@ -91,6 +91,25 @@ void SHOCMacrophysics::set_grids(const std::shared_ptr<const GridsManager> grids | |||
| add_field<Computed>("ustar", scalar2d, m/s, grid_name, ps); | |||
| add_field<Computed>("obklen", scalar2d, m, grid_name, ps); | |||
|
|
|||
| // Extra SHOC output diagnostics | |||
| if (m_params.get<bool>("extra_shoc_diags", false)) { | |||
There was a problem hiding this comment.
While this option makes sense to avoid polluting the FM with stuff we don't need, it has the drawback that users must atmchange this whenever they add one of these fields to their output yaml files.
I suppose this is the best we can do for now. Hopefully this will become obsolete once we rework a bit the IO layer for optional outputs.
| if (m_params.get<bool>("extra_shoc_diags", false)) { | ||
|
|
||
| const auto& shoc_mix = get_field_out("shoc_mix").get_view<Spack**>(); | ||
| Kokkos::deep_copy(shoc_mix,history_output.shoc_mix); |
There was a problem hiding this comment.
This is indeed costly, for no reason. In principle, we would like history_output.shoc_mix to simply use the view stored inside the Field "shoc_mix". We are basically adding 10 3d fields for no real reason...
OTOH, this may add more complexity, since we need to be careful during init, depending on the value of extra_shoc_diags:
- false: request enough buffer mem to accommodate these vars.
- true: do NOT request buffer mem for these fields, and grab them from the computed field during initialize_impl
I still think keeping memory under control in this case is doable with not too much effort...
There was a problem hiding this comment.
The buffer needs a bit of a reworked to allow for it to be dynamically sized based on a param and not be unreadable. @bogensch If you do not want to do that I can take a look. But at the very least we can get rid of the deep copies like Luca said, in initialize_impl:
if(m_params.get<bool>("extra_shoc_diags")) {
history_output.shoc_mix = get_field_out("shoc_mix").get_view<Spack**>();
} else {
history_output.shoc_mix = m_buffer.shoc_mix;
}
There was a problem hiding this comment.
Not too much logic to add. When SHOCMacrophysics::requested_buffer_size_in_bytes and SHOCMacrophysics::init_buffers are called, we can ping m_params, to see if we need the extra mem for those fields or not.
There was a problem hiding this comment.
You're right, it's not too much logic. But we use a constexpr for number of fields in the buffer, and then we initialize the buffer views in a way that probably doesn't lend itself to using if-else nicely. So I think some structural tweaks will need to happen.
There was a problem hiding this comment.
@tcclevenger what's your plan regarding these unnecessary copies? Are you planning to fix them here or in another PR? If the latter, you can go ahead and merge, since the cosp failures are orthogonal.
There was a problem hiding this comment.
I'll merge and include this fix in new diag output work.
|
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_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bogensch |
|
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_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5794 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 6020 PASSED (click to see last 100 lines of console output) |
E3SM-Autotester
removed
the
AT: RETEST
|
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_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: bogensch |
|
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_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Mappy # 5804 FAILED (click to see last 100 lines of console output)SCREAM_PullRequest_Autotester_Weaver # 6030 PASSED (click to see last 100 lines of console output) |
| if (m_params.get<bool>("extra_shoc_diags", false)) { | ||
|
|
||
| const auto& shoc_mix = get_field_out("shoc_mix").get_view<Spack**>(); | ||
| Kokkos::deep_copy(shoc_mix,history_output.shoc_mix); |
There was a problem hiding this comment.
@tcclevenger what's your plan regarding these unnecessary copies? Are you planning to fix them here or in another PR? If the latter, you can go ahead and merge, since the cosp failures are orthogonal.
Adds the option to output several SHOC related variables.