-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add explicit layerThicknessEdge variables #12
Add explicit layerThicknessEdge variables #12
Conversation
In a few places, we trigger the same behavior as for the CAM macro.
EAM's functions are just wrapping share_file_mod's ones, without adding any logic. This way, we can use the module even if EAM is not used (e.g., for SCREAM builds). Also, fixed indentation of some if blocks in namelist_mod.F90
This can happen for SCREAM, where dt is not known at the place where the initialization happen. It will instead be known only when timestepping is performed. To be safe, add a check at the beginning of prim_run_subcycle, to ensure nsplit has been set to a value>=1.
… a noinline clause to the FFT init to fix it.
! layerThicknessEdge is computed in compute_solve_diag, and is not available yet, | ||
! so recompute layerThicknessEdge here. |
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.
Noting that this statement is false. layerThicknessEdge
is computed in ocn_diagnostic_solve,
which is called for timeLevel=2
in each time integration scheme before ocn_vel_vmix_tend
is called.
87904b9
to
d0389d9
Compare
@cbegeman, and others, a general question for discussion. Are we okay with |
@xylar I can change it to |
Yes, I think I like |
I very much like |
Just stepping back from the naming conversation for a moment, I'd say the main reason we want to support upwind-type I think the main terms we have to worry about are the To aim for a minimal change, keeping all other forcing, friction and parameterisation tendencies using the existing centred scheme seems like a good place to start to me. In terms of |
@dengwirda Thanks for posting this context. I had in mind that this PR would simply introduce the new variables and aim for correctness in designating which of these variables should be deployed in which terms. To my knowledge folks aren't utilizing the |
@cbegeman Yes, agree all round --- I feel that using |
@dengwirda Got it. Commits c731e62 and 454069f revert my changes to horizontal mixing and cvmix so they will use the centered thicknesses. |
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 looks like there are still a few changes related to the move from layerThciknessEdgeCenter
to layerThicknessEdgeMean
. I'll take a more in-depth look early next week.
Did the allocation issue get resolved or is that something I should help look into?
components/mpas-ocean/src/shared/mpas_ocn_diagnostics_variables.F
Outdated
Show resolved
Hide resolved
components/mpas-ocean/src/shared/mpas_ocn_diagnostics_variables.F
Outdated
Show resolved
Hide resolved
components/mpas-ocean/src/shared/mpas_ocn_diagnostics_variables.F
Outdated
Show resolved
Hide resolved
Upgrading to the latest YAKL. Includes performance improvements for CUDA and HIP as well as a bug (memory leak) fix for the pool allocator. The new YAKL also has a fully functioning SYCL backend for Intel GPUs. CUDA has what appears to be a compiler bug in the FFT routines. These have been worked around in the new YAKL. The FFT API changed by no longer requiring a parameter to init(). This has been reflected in pressure.cpp and crm_variance_transport.cpp An issue with the standalone samxx has been fixed, and new machine files have been added. This PR passes ./create_test SMS_Ld10_P6x7.ne4pg2_ne4pg2.F-MMFXX.summit_gnugpu.eam-rrtmgpxx [BFB]
@cbegeman thanks for your detailed work here, and apologies for the delayed reponse. I agree with these changes. Having The global stats names follow the previous patterns so I agree with your choices, even though I do worry about the memory footprint of MPAS-Ocean. We add 3D arrays freely, so the memory footprint becomes quite bloated. I'm guilty of that too. For example, it appears that For the purpose of having a separate variable for the flux quantity for the layer thickness, defined with flags at the correct place, this all looks right to me. |
@mark-petersen Thanks for giving this a thorough look!
I personally don't see either of these variables being particularly useful from a global stats perspective because they are going to reflect the global stats for To your question about whether an additional 3d array is worth it: I guess the short answer is that we'd have to assess this further. If we're going to go this route, my first inclination would be to consider making Should we try to flag these issues for further consideration, with a github issue or somewhere in the code? |
OK, since you agree, please take The decision between permanently allocated arrays and flops is a tricky one. Flops are free, as they say, but retrieval of variables from memory takes much longer. Here we can either retrieve one column of I think So my conclusion is, go ahead and leave those two variables as-is. Thanks! |
You'll probably regret tagging me since this one hits two of my biggest pet peeves with MPAS and you're catching me after a long day of virtual workshop. So... (1) Registry must die and (2) if you've heard of software containers, ocean diagnostics is a software frickin' dumpster and adding any more to it is contributing to the dumpster fire that will result. Can we please start properly scoping variables? That's not directed at any of you specifically, just that we've collectively gotten lazy on this. But in answer to the specific question, I'm not sure how the new edge thickness will be used so it's hard for me to say what's best. As a general rule, something as simple as this should be kept local and recomputed (even as a local scalar) rather than stored. But if it's used in more than a few places or needs to be consistent (like the edge flux) then it makes sense to compute once and share. Or if you're going to introduce options that require branching every time you use it, then computing once to avoid excessive branching might be appropriate. And if we're going to have both of these as shared variables, then maybe we need a thickness module so they can be explicitly use'd and dependencies are clear rather than continuing to add to the diagnostics clutter. |
Funny - I had a rant /rant with brackets around much of that first paragraph but this GitHub editor interpreted it as formatting and left it off... |
This is a combination of 17 commits. OpenACC changes for time integration Changing pgi-summit compile flags for OpenACC Adding scalar get_config routines to simplify OpenACC changes Adding GPU vector reconstruct routines Adding varinp_ scalar names as alternative to config_ input variable pointer names Cleaning up unused use statements in EOS Changing input coeff in EOS from pointer to scalar Starting to pull openacc directives out of diagnostics into time integration routines Changing diagnostics variables to create rather than copyin Moving some data movement out of diagnostics Moving more openacc data movement directives out of diagnositics routine Moving more OpenACC data movement directives out of diagnostics Moving rest of OpenACC copyins out of diagnostics Moving OpenACC data copyout movement directives out of diagnostic routine Moving more OpenACC data movement out of diagnostics Moving more OpenACC data movement directives out of diagnostic solve Moving more OpenACC data movement directives out of diagnostic solve Moving more OpenACC data movement directives out of diagnostic solve Adding use of varinp scalar variable in diagnostics routine to simplify OpenACC mods Extending out OpenACC region in time integration Extending OpenACC region in time integration routines Extending OpenACC region to include normal Barotropic/Baroclinic Velocity calcs Extending OpenACC region above Freq calcs Moving more OpenACC data directives out of diagnostics Moving remaining copyouts/updates out of diagnositic solve
@philipwjones @mark-petersen Thanks for this discussion. It seems to me that this change to where Regarding this next PR, I imagine that a separate thickness module would be appropriate so that we only compute |
…ject#4825) Fix non-standard declarations of 8-byte integers in ESMF in MPAS-Framework This PR fixes non-standard declarations of 8-byte integers in ESMF_BaseMod.F90. See the original PR for MPAS at MPAS-Dev/MPAS-Model#959. The ESMF_BaseMod module contains several non- standard declarations of 8-byte integer variables. These were identified using the GNU and NAG compilers. [BFB]
e950cfe
to
2071b66
Compare
Changes: * add crusher node in config_machines.xml, config_batch.xml * create gnu_crusher.cmake * for testing amdclang and crayclang, cmake configurations for the compilers are also added * add CPRAMD macro in shr_infnan_mod.F90.in to use ieee arithmetic
Add gnu compiler support on Crusher and AMD. Changes: add crusher configuration in config_machines.xml, config_batch.xml create gnu_crusher.cmake for testing amdclang and crayclang, cmake configurations for the compilers are also added add CPRAMD macro in shr_infnan_mod.F90.in to use ieee arithmetic [BFB]
Extending OpenACC diagnostics up to time integration Pulling OpenACC data movement directives up into top-level time integration routines and extending OpenACC region by: * Changing diagnostics variables to create rather than copyin * Changing pgi-summit compile flags for OpenACC * Adding scalar get_config routines to simplify OpenACC changes * Adding GPU vector reconstruct routines * Adding varinp_ scalar names as alternative to config_ input variable pointer names * Cleaning up unused use statements in EOS * Changing input coeff in EOS from pointer to scalar [BFB]
@cbegeman, we chatted today and @philipwjones and @mark-petersen are fine with this moving over to E3SM when you're ready. |
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 these changes. I think it is ready for an E3SM PR. Thanks @cbegeman!
2071b66
to
034b20f
Compare
Moved to E3SM-Project#4832 |
cee/15.0.0 with GPU MPI buffers can crash in a system lib like this: #4 0x00007fffe159e35b in (anonymous namespace)::do_free_with_callback(void*, void (*)(void*)) [clone .constprop.0] () from /opt/cray/pe/cce/15.0.0/cce/x86_64/lib/libtcmalloc_minimal.so.1 #5 0x00007fffe15a8f16 in tc_free () from /opt/cray/pe/cce/15.0.0/cce/x86_64/lib/libtcmalloc_minimal.so.1 #6 0x00007fffe99c2bcd in _dlerror_run () from /lib64/libdl.so.2 #7 0x00007fffe99c2481 in dlopen@@GLIBC_2.2.5 () from /lib64/libdl.so.2 #8 0x00007fffea7bce42 in _ad_cray_lock_init () from /opt/cray/pe/lib64/libmpi_cray.so.12 #9 0x00007fffed7eb37a in call_init.part () from /lib64/ld-linux-x86-64.so.2 #10 0x00007fffed7eb496 in _dl_init () from /lib64/ld-linux-x86-64.so.2 #11 0x00007fffed7dc58a in _dl_start_user () from /lib64/ld-linux-x86-64.so.2 #12 0x0000000000000001 in ?? () #13 0x00007fffffff42e7 in ?? () #14 0x0000000000000000 in ?? () Work around this by using cee/14.0.3.
Update Icepack warnings calls, cori-haswell pes
This PR replaces the existing
layerThicknessEdge
variable with two variables:layerThicknessEdgeMean
refers to the mean of the neighboring cell-centeredlayerThickness
, andlayerThicknessEdgeFlux
refers to thelayerThicknessEdge
used to flux momentum and tracers. Whenconfig_thickness_flux_type = 'centered'
layerThicknessEdgeFlux = layerThicknessEdgeMean
, but whenconfig_thickness_flux_type = 'upwind'
layerThicknessEdgeFlux
is upwinded.Prior to this PR, when
config_thickness_flux_type = 'upwind'
layerThicknessEdge
was upwinded everywhere. This PR useslayerThicknessEdgeFlux
for horizontal advection terms andlayerThicknessEdgeMean
elsewhere, such as for mixing parameterizations. This change is in preparation for adding additional options for thickness fluxes.This PR also removes
layerThicknessEdge
from global stats, since it did not offer much information not given by the global stats forlayerThickness
.