Giter Club home page Giter Club logo

marbl's People

Contributors

klindsay28 avatar mnlevy1981 avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

marbl's Issues

Generalize saved_state API

Right now, marbl_saved_state_type looks like

  type, public :: marbl_saved_state_type
     real (r8), allocatable :: ph_prev_col(:)          ! (km)
     real (r8), allocatable :: ph_prev_alt_co2_col(:)  ! (km)
     real (r8), allocatable :: ph_prev_surf(:)         ! (num_elements)
     real (r8), allocatable :: ph_prev_alt_co2_surf(:) ! (num_elements)
   contains
     procedure, public :: construct => marbl_saved_state_constructor
  end type marbl_saved_state_type

We need to generalize this in a way similar to what we have done with the diagnostics - remove the hard-coded field names in favor of something where we can easily add new variables that need to be saved from time step to time step when needed. This lets the driver allocate memory at runtime for the saved state variables, which has two main benefits:

  1. Additional saved state fields can be added in MARBL without requiring any updates to the driver
  2. The driver does not need to know which fields are being saved from time-step to time-step, so this removes some un-necessary semantic content from the driver.
    * Note that this has one drawback: the GCM will write a generic "saved_state" field to restart files, so we may want to include a way for MARBL to verify that the indexing has not changed from one run to the next.

Apply land_mask to all tracers at once

Currently, ecosys_driver_init_tracers_and_saved_state() applies the land mask to ecosys tracers, whie ecosys_driver_ciso_init_tracers() applies land mask to ciso tracers. Why not apply land mask all at once after initializing all tracers?

Required surface forcing fields?

Are there any forcing fields that are required by MARBL? (Or perhaps required in specific configurations?) If so, we should check to make sure their corresponding indices are non-zero at the end of marbl_mod::marbl_init_surface_forcing_fields (after all the field definitions).

Require marbl_status_log in drtsafe()

FIXME comment in marbl_co2calc_mod::drtsafe:

make marbl_status_log required - this is currently needed
since abio_dic_dic14_mod is calling this routine but has
not itself been MARBLized yet

Error handling

We want to pass error codes out of MARBL so that the driver can handle them. There is already a datatype in marbl_interface_types.F90:

  type, public :: marbl_status_type
     integer :: status
     character(marbl_str_length) :: message
  end type marbl_status_type

And it is used in a few places (e.g. marbl_ecosys_init_nml) but it needs to be implemented more widely.

Consistency required in naming subroutines

We are, in different modules, referring to the surface forcing is surface, sflux, and forcing. Rather than have three different terms for the same thing, we need one term used everywhere. Keith's vote is to change them all to surface_forcing, because it is accurate and descriptive.

General interior_forcing datatype and workflow

Two examples of 3D fields that are read from files and used in computing the interior tendencies are iron sediment flux and tracer restoring (but there probably are other uses and / or other uses coming in future developments). We (by which I mean "@klindsay28 and I") talked about a general set up where MARBL can request that the driver reads in these 3D fields without the driver knowing what the fields will be used for. Using phosphate restoring as an example, the workflow would look something like this:

  1. Namelist flags would indicate to marbl_init that PO4 should be restored to a climatology read from PO4_clim_var in PO4_clim_file, so MARBL will populate an element in the interior_forcing array containing the necessary metadata
  2. ecosys_driver_init will loop through the entire interior_forcing array (including our specific element containing the PO4 restoring information) and read the files into a global array.
  3. ecosys_driver_set_interior will send the correct column of the global data to marbl_set_interior
  4. marbl_set_interior will recognize that the index in the interior forcing array corresponding to phosphate restoring is non-zero, so it will call compute_interior_restore_tendency with the PO4 tracer value and the climatology passed through from the driver

So marbl_init would have a section of code that looks like

if (PO4_restore) then
  call interior_forcing_fields%add_forcing_field(field_source='PO4_clim_file', &
        field_varname='PO4_clim_var', id=tracer_restore_ind(PO4_ind))
end if

and marbl_set_interior would have

do nt=1,ecosys_used_tracer_cnt
  if (tracer_restore_ind(nt).ne.0) then
    call marbl_restore(...)
  end if
end do

Again, all the PO4_* variables are just placeholders - I think we actually have arrays of these variables (of length ecosys_used_tracer_cnt) coming in via the ecosys_restore_nml namelist.

Create template for pull requests and commits

A template for pull requests would let us provide developers with a check-list to make sure the code has been properly tested. Things to include in check list:

  • Is code bit-for-bit with previous tag?
  • Does pull request address an issue ticket? If so, which one?
  • Has the MARBL API changed?
  • What testing has been done?
  • Make sure code meets with standards outlined in design document / wiki

Make use of marbl%set_sflux and marbl%set_interior

Now that we are doing things in a column-by-column manner rather than level-by-level, we can move the contents of marbl_ecosys_set_sflux and marbl_ecosys_set_interior to marbl_set_surface_flux and marbl_set_interior, respectively. (Also, see Issue #5 regarding marbl_set_surface_flux.)

Units for MARBL

Right now MARBL has adopted the cgs unit convention from POP, but we want to enforce mks. This will require conversion of intent(in) and intent(out) in the driver, and will also requirement conversion in the diagnostics.

Rename the instance of marbl_interface_class

In the driver, POP currently declares

  type(marbl_interface_class) :: marbl

This makes searching for anything related to the instance difficult. Perhaps

  type(marbl_interface_class) :: marbl_instance

is more descriptive? And, while we're at it, maybe marbl_instance_class instead of marbl_interface_class?

Add global mean (or sum)

This is the first operation we've come across that violates the "single column" aspect of MARBL - cases where MARBL may need the driver to compute global integrals and return the values. @klindsay28 and I discussed it this afternoon, and think the following are all reasonable:

  1. At initialization, a GCM that is willing / able to perform a global sum should set some logical (named something like gcm_global_sum or gcm_can_compute_global_sum) indicating it has this ability
    • if this logical is .false. and MARBL is configured in a way that requires global sums, it will report an error
  2. If a global sum is requested, the entire field being summed should be added to saved state. In a restart run, the global field should be read and then the sum can be calculated (rather than just writing the sum to restart) -- this will allow us to sum fields that are needed in saved state for other purposes
    • One possibility that was floated was making global_sum an option in the upcoming marbl_saved_state_type; by default, we would not sum saved state fields but to get a global sum you add a field to saved state and then request the sum

Generalize surface forcing output driver

marbl_surface_forcing_output_type is currently hardcoded to only output flux_o2 and flux_co2; this needs to be generalized (and totalChl added to the type). Adding totalChl will allow us to move ecosys_driver_compute_totalChl into MARBL.

Duplicate locations of some constants?

We had copies of c0, c1, c2, c1000, and p5 in marbl_kinds_mod.F90, and I am in the process of moving them to a new marbl_constants_mod.F90 module, but it appears they are also stored in marbl_parms.F90 Obviously these only need to be stored in one place, but which module to use should depend on what other constants / parameters we end up saving and where we stick the parameters (will there be a marbl_params_type inside marbl_interface_class?)

Consistent file names

We want module XYZ in XYZ.F90 (and module ABC_mod in ABC_mod.F90). I'm anticipating the following file name changes when we are ready:

$ mv ecosys_diagnostics_mod.F90 marbl_diagnostics_mod.F90 
$ mv ecosys_mod.F90 marbl_mod.F90
$ mv ecosys_restore.F90 ecosys_restore_mod.F90 
$ mv marbl_kinds.F90 marbl_kinds_mod.F90
$ mv marbl_share.F90 marbl_share_mod.F90

Should MARBL abort when mass balance checks fail?

We compute 100m vertical integrals in cases where we also compute the full-depth vertical integral and expect the latter to be 0 to round-off level... instead of returning these integrals as diagnostics, should MARBL check to make sure the full-depth integral is less than some threshold and error out if the threshold is exceeded?

marbl_ciso_set_interior_forcing updates DIC_loc

The variable marbl_interior_share is an intent(inout) in marbl_ciso_mod::marbl_ciso_set_interior_forcing with a comment saying

FIXME - intent is inout due to DIC_loc

It looks like both DIC_loc and DOC_loc are set to 0 below the deepest ocean layer; maybe we can ensure these variables are prior to calling the routine? Or only update values for layers 1:column_kmt?

Error checking in marbl_restore_mod

marbl_restore_mod::init should verify that the number of non-empty elements of restore_short_names and restore_filenames are the same, otherwise we are either requesting to restore a tracer without providing a climatology or providing a climatology without associating it with a specific tracer.

Sort out tracer indexing issues

In some places CISO tracers are indexed from 1:ecosys_ciso_tracer_cnt and in others they are ecosys_tracer_cnt+1:ecosys_tracer_cnt+ecosys_ciso_tracer_cnt; we want to use the latter in all instances and rely on the ciso_on flag to determine whether to do CISO computations or not.

MARBL does not need both dz and delta_z

We should remove dz from marbl_domain_type and all layer depth references should use delta_z (POP has a 1D array for using the same levels throughout the model and a 3D array for partial bottom cells; MARBL does not need to differentiate between these two.)

Want to print entire namelist to log

Currently, the GCM reads four MARBL namelists (ecosys_nml, ecosys_ciso_nml, ecosys_parms_nml, and ecosys_restore_nml) from a file and passes them as a string to MARBL. MARBL sets default values, then updates those values based on what the GCM passes.

Desired behavior: MARBL then writes the entire namelist to status log
Current behavior: MARBL writes the values set in the namelist file to status log

We need a build-namelist tool

How do the GCMs produce namelists to be sent to MARBL? Does it make sense to have a build-namelist utility in MARBL? One option would be to produce XML files for definitions / default values as is done in CESM / ACME components.

Updates for marbl_restore_mod

As discussed at the Feb 29 meeting:

  1. The tracer_restore function should be renamed compute_interior_restore
  2. Can linear interpolation for determining inv_tau use the same interpolation routine as the diagnostics (which interpolate to find saturation depth for calcite and aragonite)?
  3. Looking up the name of a tracer based on index should be a separate function (perhaps in a new module named marbl_utils_mod.F90)

Note that work towards these issues should keep the big picture of Issue #14 in mind; it may not make sense to do the last task in this list (tracer index look-up routine) since it may not be needed once the interior restore process is more generalized. I think it will prove to be useful, though, since we still need to turn elements in the restore_short_names namelist array into integers corresponding to the correct tracer index.

Eventually move ciso_on into marbl_interface_class

We discussed today the idea of a marbl_init_type datatype, that would be set in marbl_instance%init() and referenced in marbl_instance%set_interior() and marbl_instance%set_surface_forcing(). It would definitely contain ciso_on, which is currently part of the driver because much of the if (ciso_on) then logic is still in the driver rather than pushed into the subroutines in marbl_interface_class. So ideally ciso_on will be part of the marbl_interface_class (which will involve moving it from ecosys_driver_nml to ecosys_nml).

Before we can do that we need to clean up the driver... which will be difficult due to the indexing scheme used by the tracers. Sometimes the CISO tracers are 1:ecosys_ciso_tracer_cnt; other times they start the indexing at ecosys_tracer_cnt + 1. Perhaps that move from the driver to MARBL should be a separate issue?

Labelling this a bug, but priority is somewhat low given the dependence on cleaning up the driver code.

What should the directory structure of this repository look like?

I have a sandbox on yellowstone where I can build libmarbl.a, but before bringing it in to this repository we need to decide what the directory structure will look like. CVMix is loosely based off the structure of HOMME -- there is a src/shared/ directory containing everything needed to build libcvmix.a, and the rest of src/ (as well as src/driver/) is used to build the stand-alone driver / test-suite. Are we going to have any stand-alone tests in MARBL? As a straw-man, I'll propose the following structure:

./ -- Contains a README file
src/ -- All Fortran code needed to build libmarbl.a as well as dependency-generator tool and a Makefile. I picture this Makefile requiring the user to pass in the compiler (as well as any compiler flags) explicitly... something like

$ make FC=gfortran FCFLAGS="-O2 -ffree-form"

driver/ -- If desired, standalone driver code could go here (along with its own Makefile which would execute make -f ../src/Makefile as part of the build)

I'd also like bld-test/ with a Makefile that lets us use different compilers with simpler targets, e.g.

$ make intel
$ make gnu

These targets would execute make -f ../src/Makefile with pre-determined FC / FCFLAGS combinations to allow for easy / automated testing.

Does vflux_flag belong in the driver or in MARBL?

Currentl vflux_flag resides in the driver and is set as follows:

    vflux_flag(:) = .false.
    vflux_flag(dic_ind) = .true.
    vflux_flag(alk_ind) = .true.
    vflux_flag(dic_alt_co2_ind) = .true.

But maybe we just want the driver to tell MARBL if it is using a virtual salt flux and then MARBL can set these flags accordingly? Where does this fall on the science vs infrastructure spectrum?

marbl_interface_types should use marbl_log_type

Several routines in marbl_interface_types print error messages directly to stdout and have comments about needing to trigger an abort... these should all be using marbl_log_error, which means they need to be passed a marbl_log_type

potential division by zero in pChl_subcol computation

The computation of pChl_subcol, in marbl_compute_autotroph_photosynthesis, is

            ! GD 98 Chl. synth. term
            pChl_subcol = autotrophs(auto_ind)%thetaN_max * PCphoto_subcol / &
                 (autotrophs(auto_ind)%alphaPI * thetaC * PAR_avg(subcol_ind))

The denominator can underflow to zero, even if the individual terms are positive. This happened in a coupled run using cesm_pop_2_1_20160222 (before MARBL was separated out). In this run, PAR_avg was less than 10^-300, in an individual sub-colum at a single grid point, but it was positive.

A work-around is to set PAR%interface to zero if it is below a threshold, after it is computed and before it is used to set PAR%avg. This is in marbl_compute_photosynthetically_available_radiation.

Update marbl_interior_forcing_input_type

There are two FIXMEs related to marbl_interior_forcing_input_type. One is in the type declaration:

FIXME: better names for PAR_col_frac and surf_shortwave

and one is in the variable declaration in marbl_interface.F90:

FIXME: make this a 3d real array with indices

I'm not sure if this data type is one that we want to generalize, or if hard-coded variable names are okay, or if we just want a flat 3d array... but we definitely want to update it somehow.

Error-checking for tracer indices

Once we have merged #48 we should add a consistency_check() routine to marbl_tracer_index_type to ensure the indices of all desired tracers are non-zero

MARBL timers?

There is a FIXME comment in marbl_mod that reads:

new marbl timers need to be implemented to turn
on timers here around this subroutine call

(subroutine call in question is marbl_compute_carbonate_chemistry). Do we want MARBL to be able to provide timing information? If so, how do we incorporate that?

Should we assume that ecosys_ind_begin = 1?

ecosys_ind_begin = 1 and ecosys_ind_end = ecosys_tracer_cnt because we call set_tracer_indices with 'ECOSYS' before we call it with 'CISO'. Throughout MARBL we are alternating between declaring vectors with dimension(ecosys_tracer_cnt) and dimension(ecosys_ind_begin:ecosys_ind_end). We need to be consistent, but it is unclear which is preferable.

Make namelist buffer size runtime configurable?

Right now we hard-code three parameters needed to pass namelists from GCM to MARBL:

marbl_nl_in_size is the number of characters in the entire namelist file
marbl_nl_cnt is the number of distinct namelists in the file
marbl_nl_buffer_size is the number of characters in the largest namelist

It would be great if there was a way to configure these values at run-time, though we need a smart way to do it. Maybe let the GCM determine them and pass values to MARBL during init?

Error-checking needed in marbl_namelist_mod

Three similar FIXME comments in marbl_namelist_mod::marbl_nl_split_string:

add error checking in acse
(i+1-old_pos) > marbl_nl_buffer_size
add error checking in case first character is not '&'

and in marbl_namelist_mod::marbl_namelist:

add error checking in case &nl_name is not found

CECO compsets have runtime error with gnu compiler

When building CESM with the gnu compiler, compsets that use the ecosystem ocean tracer module produce the following runtime error:

     (passive_tracer_tools:read_field_3D) reading alk_riv_flux from /glade/p/cesmdata/cseg/inputdata/ocn/pop/gx1v6/forcing/river_nutrients_GNEWS2000_gx1v6.nc
     (passive_tracer_tools:read_field_3D) reading doc_riv_flux from /glade/p/cesmdata/cseg/inputdata/ocn/pop/gx1v6/forcing/river_nutrients_GNEWS2000_gx1v6.nc
 (tavg_requested) id =            0
     (tavg_requested) FATAL ERROR: invalid tavg id
------------------------------------------------------------------------

POP aborting...
 FATAL ERROR: invalid tavg id

------------------------------------------------------------------------

The error does not crop up when using the Intel or NAG compilers.

Add support for timing MARBL routines

Jim Edwards suggests creating a timing module (marbl_timing_mod.F90) and using #ifdef wrappers to support GPTL if requested at build time (otherwise we can fall back to walltime or something). This is already done (in a way) in POP & CIME; POP uses CCSMCOUPLED in source/timers.F90 to trigger calls to cime/share/timing/perf_mod.F90 which is just a wrapper for GPTL. My first thought is to cut out the middle man and call GPTL directly if it is available.

This is not currently a high priority, but I would like to finish it sometime this summer... I believe it'll be useful to have #16 finished, because that bugfix will also provide the framework for a stand-alone driver and we'll be able to do some initial testing outside of CESM before trying to tie the MARBL timers in to POP.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.