CVMix / CVMix-src

CVMix source code (including protex documentation) as well as build system and examples / tests
Other
30 stars 30 forks source link

Two public functions added to get the enhancement factor for Langmuir mixing. #67

Closed qingli411 closed 8 years ago

qingli411 commented 8 years ago

The function cvmix_kpp_efactor_read reads the monthly enhancement factor from a file and interpolates it to a particular location and time.

The function cvmix_kpp_efactor_model calculates the enhancement factor from 10-meter wind, surface friction velocity and the boundary layer depth.

Either one could be used to get the enhancement factor before passing it to subroutine cvmix_kpp_compute_turbulent_scales or subroutine cvmix_coeffs_kpp

mnlevy1981 commented 8 years ago

I have one issue with this pull request, but it's kind of a large one -- we don't want CVMix to do any file IO, so we wouldn't want cvmix_kpp_efactor_read() included in the library. A better option would be to have the GCM read the data and pass it to CVMix -- the thinking behind this policy (which I didn't see spelled out in the Developer's Guide, apologies for that oversight) is two-fold, both related to the fact that we build CVMix without any external dependencies:

  1. CVMix isn't MPI-aware, so if you are running on a large number of tasks each task will try to read from disk (having POP do the file read, for example, will let us use the PIO library for better parallel input)
  2. CVMix isn't built with netCDF, so you are limited to just reading binary data files. Having the GCM do the file read and passing the data to CVMix allows for more options when it comes to file format.

We intentionally do not require netCDF or MPI when building CVMix to keep it light-weight. I think this pull request should only include cvmix_kpp_efactor_model().

qingli411 commented 8 years ago

OK, I see. The purpose of cvmix_kpp_efactor_read() is just to offer a quick option for users to easily test the effect of Langmuir mixing without doing much coding on reading and interpolating the data. I agree that for production purpose it's better to read the data in the ocean model outside CVMix. I'm OK with that we only include cvmix_kpp_efactor_model() at this moment. Thanks a lot!

Best, Qing

在 2016年6月24日,09:28,Michael Levy notifications@github.com 写道:

I have one issue with this pull request, but it's kind of a large one -- we don't want CVMix to do any file IO, so we wouldn't want cvmix_kpp_efactor_read() included in the library. A better option would be to have the GCM read the data and pass it to CVMix -- the thinking behind this policy (which I didn't see spelled out in the Developer's Guide, apologies for that oversight) is two-fold, both related to the fact that we build CVMix without any external dependencies:

CVMix isn't MPI-aware, so if you are running on a large number of tasks each task will try to read from disk (having POP do the file read, for example, will let us use the PIO library for better parallel input) CVMix isn't built with netCDF, so you are limited to just reading binary data files. Having the GCM do the file read and passing the data to CVMix allows for more options when it comes to file format. We intentionally do not require netCDF or MPI when building CVMix to keep it light-weight. I think this pull request should only include cvmix_kpp_efactor_model().

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

qingli411 commented 8 years ago

The function cvmix_kpp_efactor_read() is removed in this version. And there are a few updates in the function cvmix_kpp_efactor_model().

mnlevy1981 commented 8 years ago

This looks much better! I have small nitpicks about everything in this block of code, though:

    real(cvmix_r8), parameter :: &
        ! gravity
        g = 9.81_cvmix_r8, &
        ! ratio of U19.5 to U10 (Holthuijsen, 2007)
        u10_to_u19p5 = 1.075_cvmix_r8, &
        ! ratio of mean frequency to peak frequency for
        ! Pierson-Moskowitz spectrum (Webb, 2011)
        fp_to_fm = 1.296_cvmix_r8, &
        ! ratio of surface Stokes drift to U10
        u10_to_us = 0.0162_cvmix_r8, &
  1. I don't think we want to hard-code the gravitational constant here, nor do I think we want it to be 9.81 (CESM uses 9.80616 m/s2); I think we want to add gravity to cvmix_global_params_type and let it come from the GCM, maybe with the 9.80616 default (I don't remember if we provide default values for other global parameters)
  2. It seems like all your ratio parameters are misnamed - I see fp_to_fm and think fp/fm but you define it as fm/fp. I'd prefer the following parameter names:
u19p5_to_u10
fm_to_fp
us_to_t10
qingli411 commented 8 years ago

OK, I agree. The gravitational constant is now in cvmix_global_params_type with a default value of 9.80616. The ratio parameters are renamed.

mnlevy1981 commented 8 years ago

This code is available as of the v0.83-beta tag