Skip to content
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 Langmuir mixing parameterization via CVMix #134

Merged
merged 3 commits into from
May 30, 2019

Conversation

qingli411
Copy link

@qingli411 qingli411 commented Jan 16, 2019

This merge adds Langmuir mixing parameterization via CVMix

  1. Change the CVMix repository to use the 'theory-wave' approximation of Li et al., 2017 to estimate the Langmuir enhancement factor from 10-meter wind, surface friction velocity and boundary layer depth
  2. New entries in mpaso namelist allowing different options of Langmuir mixing parameterization

MPAS-Analysis:

G-case without Langmuir turbulence parameterization

config_cvmix_kpp_langmuir_mixing_opt = 'NONE'
config_cvmix_kpp_langmuir_entrainment_opt = 'NONE'

G-case with Langmuir (LWF16)

config_cvmix_kpp_langmuir_mixing_opt = 'LWF16'
config_cvmix_kpp_langmuir_entrainment_opt = 'LWF16'

G-case with Langmuir (LF17)

config_cvmix_kpp_langmuir_mixing_opt = 'LWF16'
config_cvmix_kpp_langmuir_entrainment_opt = 'LF17'

B-case with Langmuir (LF17, 40 years)

config_cvmix_kpp_langmuir_mixing_opt = 'LWF16'
config_cvmix_kpp_langmuir_entrainment_opt = 'LF17'

1. Change the CVMix repository to use the 'theory-wave' approximation of Li et al., 2017
to estimate the Langmuir enhancement factor from 10-meter wind, surface friction velocity
and boundary layer depth
2. New entries in mpaso namelist allowing different options of Langmuir mixing parameterization
Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qingli411 this looks good. I have just two requests. Let me know if you have any questions

@@ -872,6 +917,9 @@ subroutine ocn_vmix_cvmix_init(domain,err)!{{{
call mpas_pool_get_config(ocnConfigs, 'config_cvmix_convective_diffusion', config_cvmix_convective_diffusion)
call mpas_pool_get_config(ocnConfigs, 'config_cvmix_convective_viscosity', config_cvmix_convective_viscosity)
call mpas_pool_get_config(ocnConfigs, 'config_cvmix_kpp_use_enhanced_diff', config_cvmix_kpp_use_enhanced_diff)
call mpas_pool_get_config(ocnConfigs, 'config_cvmix_kpp_langmuir_mixing_opt', config_cvmix_kpp_langmuir_mixing_opt)
call mpas_pool_get_config(ocnConfigs, 'config_cvmix_kpp_langmuir_entrainment_opt', &
config_cvmix_kpp_langmuir_entrainment_opt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a check below that user input values are acceptable values of your two character strings. You can follow L1022 and following for an example check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanroekel Added.

else
langmuirEnhancementFactor = 1.0_RKIND
end if

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this down after L651, you may miss some boundary layer changes in your computation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanroekel Done.

@vanroekel
Copy link
Contributor

@mark-petersen two questions on this PR

  1. this should be BFB with ocean/develop as this is off by default and will be climate changing if on. How do you want to test this for merging to MPAS-Model? @qingli411 has run a core cycle with these changes and 40 years of a B-case. Results are as expected.

  2. is it okay to merge this even though we have to point to @qingli411 personal cvmix repo? He has issued a PR into cvmix for the changes. Also asking @jonbob

1. Check the values of parameters 'config_cvmix_kpp_langmuir_mixing_opt' and
 'config_cvmix_kpp_langmuir_entrainment_opt'
2. Move the lines that update Langmuir enhancement factor after the update
 of boundary layer depth
@mark-petersen
Copy link
Contributor

mark-petersen commented Jan 25, 2019

@qingli411 and @vanroekel thanks for your work here.

Please conduct a bfb comparison in MPAS-Ocean that exercises your code options, for two tests:

  1. exact restart
  2. comparison across different partition numbers

that will catch any issues with initialization, restart fields, and halo updates. I know the odds of those mistakes are low on a new cvmix option, but these are easy tests to do. You can use six hour tests at QU240, which are in COMPASS.

cd testing_and_setup/compass/
./list_testcases.py |grep restart
      113: -o ocean -c global_ocean_V1 -r QU240 -t restart_test
./list_testcases.py |grep blocks
      115: -o ocean -c global_ocean_V1 -r QU240 -t se_blocks_test
./setup_testcase.py -f general.config.ocean_turq --work_dir DIR -n 115

etc. Or you can just use your standard test right now, but write a restart file at the end and use ncdiff for comparison.

Also run your standard test with both a gnu and intel compiler in debug mode, which will catch things you don't see without debug.

@mark-petersen
Copy link
Contributor

Also good to run a performance test to catch anything major. Run your standard test with standard cvmix and your new option. Compare the vertical mixing timers at the end of the ocean log file. We should know if time is much larger. This is the time to catch major performance changes, like loops nested in the wrong order.

CVMIX_GIT_HTTP_ADDRESS=https://github.com/qingli411/CVMix-src.git
CVMIX_GIT_SSH_ADDRESS=git@github.com:qingli411/CVMix-src.git
CVMIX_SVN_ADDRESS=https://github.com/qingli411/CVMix-src/tags
CVMIX_WEB_ADDRESS=https://github.com/qingli411/CVMix-src/archive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use @qingli's repo here, as then all mainline E3SM simulations would depend on a nonstandard CVMix tag. Worse, we need to be able to go back to any previous E3SM/master commit and run it in future years. That means all submodules also need to be permanent.

The solution is to simply not make the above changes. We can still merge this PR. In order to run with @qingli411's non-default option, one would need to make the above change with your local E3SM repo copy.

After Qing's changes are merged into CVMIX, we make an MPAS PR to update the above CVMIX_TAG line.

@qingli411
Copy link
Author

@vanroekel and @mark-petersen, here are some updates on this PR:

  1. My changes to CVMix have been merged into CVMix with tag v0.94b-beta. I have updated the git repo of CVMix in this PR.

  2. I've conducted two tests (six hour tests at QU240 in COMPASS) on Grizzly with target 'ifort-gcc': 1) exact restart and 2) comparison across different partition numbers, which are all passed.

  3. I've also compared the vertical mixing timers in these two tests and the timers are not significantly different from each other (within 3%). But I haven't tried comparing the timers in a longer run.

@qingli411
Copy link
Author

I just checked the vertical mixing timers (vmix imp) in my G test cases listed above. It takes 6.9% and 13.8% longer than the standard CVMix for vertical mixing with the first (LWF16) and the second (LF17) options, respectively, in a 5-year run. This is closer to what I was expecting, as there are additional computations in the second option and it should take longer time than the first option.

@vanroekel
Copy link
Contributor

@qingli411 could you also include the percentage increase in the overall ocean timestep timer? I can't remember how much of the timestep is CVMix.

@qingli411
Copy link
Author

@vanroekel For ocean timestep timer ('o:se timestep') it's 0.16% and 0.64% increase respectively for the two options.

@vanroekel
Copy link
Contributor

@mark-petersen is there anything @qingli411 and I can do to move this PR along? It should be ready to go.

@mark-petersen
Copy link
Contributor

I am merging in changes that are bfb on all E3SM tests. See order here:
#229 (comment)
Should this one go in now? I would put it at the end.

@mark-petersen
Copy link
Contributor

@vanroekel OK, just saw your comment. Will test and merge.

@vanroekel
Copy link
Contributor

It is fine to wait till after the BFB PRs in #229. Since this is climate changing it should probably go on its own and on its own to E3SM.

Copy link
Contributor

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged into ocean/develop locally. Tested on grizzly, gnu/intel, opt/debug, nightly regression suite. Passes all. Merging in.

@mark-petersen mark-petersen merged commit 11c949d into MPAS-Dev:ocean/develop May 30, 2019
@qingli411 qingli411 deleted the langmuir_mixing branch May 4, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants