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

Do we need local versions of config flags? #394

Open
mark-petersen opened this issue Nov 12, 2019 · 3 comments
Open

Do we need local versions of config flags? #394

mark-petersen opened this issue Nov 12, 2019 · 3 comments

Comments

@mark-petersen
Copy link
Contributor

In almost every MPAS-Ocean module, config flags are transferred to private module variables. For example, in the ocean core, for del2, we have:

   ! Private module variables
   logical :: del2On
   real (kind=RKIND) :: eddyDiff2

and on init:

   subroutine ocn_tracer_hmix_del2_init(err)!{{{
...
      logical, pointer :: config_use_tracer_del2
      real (kind=RKIND), pointer :: config_tracer_del2
...
      call mpas_pool_get_config(ocnConfigs, 'config_use_tracer_del2', config_use_tracer_del2)
      call mpas_pool_get_config(ocnConfigs, 'config_tracer_del2', config_tracer_del2)

      del2on = .false.

      if ( config_use_tracer_del2 ) then
         if ( config_tracer_del2 > 0.0_RKIND ) then
            del2On = .true.
            eddyDiff2 = config_tracer_del2
         endif
      endif

Is there any performance reason to do this? It seems like a lot of extra bookkeeping, and extra variables obscure code. Options are:

  1. Do it this way (use extra variables with different names)
  2. Call mpas_pool_get_config for config flags every time the main routine is called (i.e. every time step)
  3. Define those config_* variables as private module variables and fill them in on init
  4. Obtain ALL config_* variables ONCE on init in some new module (say ocn_configs) and then have every module use ocn_configs at the top and use config variables by name. Then we would never declare config flags or call mpas_pool_get_config within these normal modules.

I'm asking now because I'm adding new flags for Redi mixing, but it is relevant to all modules. Any of 1-3 we could alter piecemeal. Number 4 (which I prefer) would need a larger coordinated change.

I remember from @philipwjones talk that evaluating strings in if statements is expensive, so there may be two cases here, one for config flags that are strings, and one for flags that are already logicals, integers, or real.

@amametjanov
Copy link
Contributor

amametjanov commented Nov 12, 2019

+1 on doing it once with option 4. Not sure if there's a reason for the current method: module-private vars still depend on run-time input namelists: i.e. they're not compile-time constant parameter vars.

@philipwjones
Copy link
Contributor

We do not want options 1,2 as they are likely causing some unnecessary performance overhead doing the character matching every time step. We definitely want to set them on init and not keep retrieving them from config.

Using local module variables (option 3) keeps most of them encapsulated within the module that's using them. And because they're encapsulated, you can give them shorter variable names with less of a risk of namespace conflict. And for multiple choices, that will keep most changes local to a module if you need to add another option and doesn't clutter up a big comprehensive config module. For variables that need to be accessed by multiple modules, you can either make them shared and include the relevant module where it's used or if that creates complicated dependencies, you could keep them in an independent module (hopefully that's rare).

Either way, please convert strings to other forms. Simple binary options should be implemented as logical flags. For conditions where we have multiple options, the preferred way would be to use the Fortran equivalent of enums - basically named integer parameters. So something like this contrived setup:
integer, parameter :: &
advectChoiceStd = 1,
advectChoiceFCT = 2,
advectChoiceSLT = 3

integer myAdvectChoice
! set myChoice based on input nml on startup

select case (myAdvectChoice)
case(advectChoiceStd)
do stuff
case(advectChoiceFCT)
do stuff
case(advectChoiceSLT)
do stuff
case default
flag error
end select

@philipwjones
Copy link
Contributor

To clarify, when I say convert strings - we can (and should) still have string options in the namelists - just want to convert them during init to something that's more efficient for the integration.

mark-petersen added a commit that referenced this issue Aug 3, 2020
…o ocean/develop

This PR replaces almost all calls to mpas_allocate_scratch_field in the
ocean model with straight Fortran allocates. This is done as part of the
MPAS framework rewrite, per @philipwjones .

This PR also replaces almost all calls to mpas_pool_get_config in
subroutines with definitions in ocn_config via use ocn_config. This
addresses #394 , and allows #406 to be closed without being merged.

There are still a few calls to mpas_allocate_scratch_field that cannot
be replaced until more of the MPAS framework rewrite has been completed
(namely removal of blocks).

This PR only changes the files in the shared subdirectory, not the
mode_forward subdirectory.
mark-petersen added a commit that referenced this issue Aug 4, 2020
…develop

This PR replaces almost all calls to mpas_allocate_scratch_field in the
ocean model with straight Fortran allocates. This is done as part of the
MPAS framework rewrite, per @philipwjones .

This PR also replaces almost all calls to mpas_pool_get_config in
subroutines with definitions in ocn_config via use ocn_config. This
addresses #394 , and allows #406 to be closed without being merged.

There are still a few calls to mpas_allocate_scratch_field that cannot
be replaced until more of the MPAS framework rewrite has been completed
(namely removal of blocks).

This PR only changes the files in the shared subdirectory, not the
mode_forward subdirectory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants