add distinct passive tracer background vert diffusivity

[maltrud]

Add capability to specify a different background vertical diffusivity for passive tracers than is used for active tracers. Design document is attached. Testing described in the design document with EC30to60E2r2 grid G-case was successful.

designDocumentPassiveTracerDiffusivity.pdf

[NML]
[BFB]

[github-actions]

PR Preview Action v1.4.7
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6263/
on branch gh-pages at 2024-05-09 21:00 UTC

[rljacob]

@mark-petersen please review.

[rljacob]

@maltrud since Mark is out of the office, can someone else review this?

[vanroekel]

@maltrud this probably doesn't impact the simulation but this does create a nonzero diffusivity at the surface and bottom of the ocean, which I don't think we want in the code. The change should only be from k=2:maxLevelCell

[mark-petersen]

I agree, this should be in explicit loops over cells as well as k=2:maxLevelCell. Implicit indexing hides everything, and does not include the acc and openmp pragmas. The loop should look like, e.g., line 1553 in mpas_ocn_vmix.F, with all the lengthy pragmas and ifdefs around it.

[maltrud]

@vanroekel and @mark-petersen thanks for finding this. I'll clean it up--array syntax has become second nature to me so going back to loops often doesn't occur to me.

[maltrud]

does this look correct to you? it compiled ok and I'll make sure it runs.

#ifdef MPAS_OPENACC
!$acc parallel loop gang vector collapse(3) present(vertDiffPassiveTopOfCell, vertDiffTopOfCell)
#else
!$omp parallel
!$omp do schedule(runtime) private(k, iTracer)
#endif
do iCell = 1, nCellsOwned
do k = 2, maxLevelCell(iCell)
vertDiffPassiveTopOfCell(k,iCell) = vertDiffTopOfCell(k,iCell) - &
config_cvmix_background_diffusion + config_cvmix_background_diffusion_passive
end do
end do
#ifndef MPAS_OPENACC
!$omp end do
!$omp end parallel
#endif

[maltrud]

should the $acc directive also have maxLevelCell?

also a general question: in the loop @mark-petersen references as an example (thanks for that!) the k loop is 1:nVertLevels, not 1:maxLevelCell(iCell). I realize this is a bit different (we don't want a k=1 value for diffusivity) but why not loop over 1:maxLevelCell?

[philipwjones]

@maltrud you won't be able to collapse these loops with maxLevelCell as a loop limit. For maximum parallelism, you can either use fixed loop (2:nVertLevels) and add a conditional inside (if k < maxLevelCell) or use a mask. Otherwise, you can have just one level of parallelism over cells and leave the loop limit variable. And yes, you'll need maxLevelCell in the present() list. You also need to change collapse to either (2) in the first case or remove it if you only parallelize the cell loop. Finally, you should verify that the arrays in the present() list have all been copied to the device at some point (all mesh related variables like maxLevel are always on device and vertDiffTopOfCell I think has been copied earlier, but don't know if you added VertDiffPassive...)

[maltrud]

thanks for your help, @philipwjones. I think I'll opt for limits of 2:nVertLevels and adding a conditional, though I assume that would affect vectorization. would using 'merge' help for vectorization? eg:

vertDiffPassiveTopOfCell(k,iCell) = merge( vertDiffTopOfCell(k,iCell) -....,0.0_RKIND,k<maxLevelCell(k,iCell))

how can I tell if something is present? I see $acc update host for vertDiffTopOfCell in ocn_vmix_coefs, then $acc update device after the coefficients are calculated. do I need to do something similar for vertDiffPassiveTopOfCell? I did put in $acc update host for vertDiffPassiveTopOfCell but I suspect that isn't correct?

[philipwjones]

@maltrud it may affect CPU/vector performance but I suspect this loop has a fairly small cost anyway. The merge probably won't be any better - this is a simple enough loop the compiler should figure it out anyway and sometimes the intrinsic is actually worse. I didn't look closely at the data transfers when I wrote that last, but what you have looks correct - this is computed on the device so you just need to update the host which you've done. And you added it to the create/destroy calls in diagnostic_vars so should be fine.

[vanroekel]

I'm guessing you meant to remove these debug lines?

[maltrud]

@vanroekel , yeah, thanks for catching that. will remove.

[vanroekel]

I took a look since Mark is out of office. I think it looks good by visual inspection, just a couple of comments.

[mark-petersen]

For multiple passive tracers this line of code is redundant. It could be placed above the tracer loop, but it is way up there, so for a few wasted flops this more clear.

[maltrud]

@mark-petersen yes I debated this one in my head a bit, and also decided clarity might might be worth some redundancy. I'll make a comment in the code but leave the logic as-is if that's OK with you.

[mark-petersen]

@maltrud, apologies for the late review. Are there instances where you would like a different background diffusion for different tracer groups, rather than a single flag for all passive tracers? That seems useful to me.

It wouldn't be too hard to make that alteration. You would change the flags config_cvmix_background_diffusion_passive and config_cvmix_background_diffusion_passive_enable to be specified for each tracer group, for example:

src/tracer_groups/Registry_CFC.xml
    <nml_option name="config_CFCTracers_background_diffusion_passive_enable" type="logical" default_value=".false."
          description="flag to enable using a different background vertical diffusion for this tracer group"
          possible_values=".true. or .false."
          />
    <nml_option name="config_CFCTracers_background_diffusion_passive" type="real" default_value="1.0e-5" units="m^2 s^-1"
          description="Background vertical diffusion applied to passive tracer quantities"
          possible_values="Any positive real value."
    />

Then you would grab that flag just before using it:

src/shared/mpas_ocn_vmix.F line 1586

         call mpas_pool_get_config(ocnConfigs, &
                 'config_use_' // groupName // '_background_diffusion_passive_enable', &
                 config_tracerGroup_background_diffusion_passive_enable)
         if (config_tracerGroup_background_diffusion_passive_enable) then

            call mpas_pool_get_config(ocnConfigs, &
                    'config_use_' // groupName // '_background_diffusion_passive', &
                    config_tracerGroup_background_diffusion_passive)

             vertDiffPassiveTopOfCell = vertDiffTopOfCell - &
                         config_cvmix_background_diffusion + config_tracerGroup_background_diffusion_passive

I think that's it, except for adding to the bld namelist scripts. Let me know what you think of that idea. Otherwise, everything looks good.

[maltrud]

@mark-petersen I don't really foresee the need for each passive tracer group to have its own value of background diffusivity.

[mark-petersen]

@maltrud, if you don't foresee the need for varying diffusivity on a per tracer basis, I'm happy with the changes you made here.

[jonbob]

@vanroekel -- can you please check and make sure your concerns have been addressed?

[vanroekel]

@jonbob it doesn't seem so. The responses to the comment look good, but they don't appear to be associated with actual code changes. Maybe I'm missing it?

[jonbob]

@vanroekel -- you're absolutely correct. We'll wait for Mat to fix this

[maltrud]

I just pushed the changes (explicit k-loop from 2:maxLevelCell(iCell)) to my branch. my test run with the changes was bit-for-bit with the case before I made the changes.

[jonbob]

passes:

• ERS.ne4pg2_oQU480.WCYCL1850NS.chrysalis_intel
• SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel

wth expected NML DIFFs. Merged to next

[jonbob]

merged to master and bless requests submitted for all expected NML DIFFs