1D Ocean Column MARBL Interface + Obs Converter #726
Conversation
…n models/MARBL_MOM6_1D in branch marbl_mom6_1d) and MARBL_joint_estimation (developed in models/MARBL_MOM6_1D on branch param_estimation).
…imation branch. Also added in the observation codes for BATS variables, which were accidentally left out of the last commit.
…r parameter smoothing.
… This required fixing a bug in direct_netcdf_mod, using a solution provided by Helen Kershaw.
…d. Split the BATS data converter into two separate converters, one for sequential state or state-parameter estimation, and another for pure parameter estimation.
… cycles with MARBL without errors. Further testing needed to assess the quality of assimilations.
…imatology.py now records observation error in units of standard deviation, and the script also generates a surrogate netCDF file containing the BATS empirical climatology.
… BATS_clim data converter.
…t, so that it works with the self-contained test package.
it is actually a netcdf file
add executables to root/.gitignore
was pointing at Robin's directory
MARBL_joint_estimation is the model_mod we are using (see pull NCAR#701)
Replaced model directory 'MARBL_joint_estimation' with 'MARBL_column' and compiled to make sure the code works
Removed the hard-coded location of BATS station and added it as `station_location` in the input.nml This allows the users to run the code for any location on earth (i.e., it doesn't have to be Bermuda station). I also cleaned up the input.nml and stripped out the old path names to Robin's directories.
The model size is now computed based on the number of active domains. Also, the accessor function get_model_size can now be used from outside the model_mod without the need for local variables. The code as it stands should be able to run: - state estimation only - parameter estimation only - state and parameters estimation
Removed unused routines and unnecessary formatting
I removed the QTYs associated with specific parameters and added a single definition: 'QTY_BGC_PARAM' The changes are now reflected in the default_quantities_mod Did some cleaning in the model-mod and changed some configuration in the input.nml file to be able to run the interpolation test
I went through all model-mod checks and made sure all tests are successful. The commit also includes some more cleaning for the model-mod and the input.nml
Updated the BATS station location in the obs_converter. Previously it assumed a lon of 64 while it should be 64W or 296. Also, the BATS text data file has been modified and that required changing the input parameters for reading the quantities properly. filter has been run several times and obs_seq.final was assessed with no apparent issues.
Added information in the readme.rst file
Removed the old readme.md file and added a new rst readme file describing the converter and its namelist options.
Added the scripts and code to generate climatology data to the BATS converter. This includes a python script and fortran code. The namelists were also consolidated. The BATS_clim converter has been removed. Documentation of both the model and the converter was also modified for clarity.
There was a problem hiding this comment.
Hi Moha,
Thanks for this. I haven't looked at the code yet.
There are some warnings in the docs build.
(py-docs) [hkershaw:DOCS]() > make html
Running Sphinx v6.2.1
/Users/hkershaw/DART/DOCS/py-docs/lib/python3.9/site-packages/urllib3/__init__.py:35: NotOpenSSLWarning: urllib3 v2 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'LibreSSL 2.8.3'. See: https://github.com/urllib3/urllib3/issues/3020
warnings.warn(
building [mo]: targets for 0 po files that are out of date
writing output...
building [html]: targets for 237 source files that are out of date
updating environment: [new config] 237 added, 0 changed, 0 removed
reading sources... [100%] theory/readme
/Users/hkershaw/DART/pull_requests/pull_726/models/MARBL_column/readme.rst:28: ERROR: Unexpected indentation.
/Users/hkershaw/DART/pull_requests/pull_726/models/MARBL_column/readme.rst:34: ERROR: Unexpected indentation.
/Users/hkershaw/DART/pull_requests/pull_726/models/MARBL_column/readme.rst:45: ERROR: Unexpected indentation.
/Users/hkershaw/DART/pull_requests/pull_726/models/MARBL_column/readme.rst:46: WARNING: Block quote ends without a blank line; unexpected unindent.
/Users/hkershaw/DART/pull_requests/pull_726/observations/obs_converters/BATS/readme.rst:20: ERROR: Unexpected indentation.
/Users/hkershaw/DART/pull_requests/pull_726/observations/obs_converters/BATS/readme.rst:56: ERROR: Unexpected indentation.
/Users/hkershaw/DART/pull_requests/pull_726/observations/obs_converters/BATS/readme.rst:79: WARNING: Inline literal start-string without end-string.
/Users/hkershaw/DART/pull_requests/pull_726/observations/obs_converters/BATS/readme.rst:97: WARNING: Title underline too short.
Climatology:
~~~~~~~~~~~
looking for now-outdated files... none found
pickling environment... done
checking consistency... /Users/hkershaw/DART/pull_requests/pull_726/models/MARBL_column/readme.rst: WARNING: document isn't included in any toctree
/Users/hkershaw/DART/pull_requests/pull_726/observations/obs_converters/BATS/readme.rst: WARNING: document isn't included in any toctree
done
preparing documents... done
writing output... [100%] theory/readme
generating indices... genindex done
writing additional pages... search done
copying images... [100%] guide/images/WRF_tutorial_evolution2.png
copying downloadable files... [100%] ../DOCS/guide/DART_LAB/presentation/DART_LAB_Section05.pdf
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 10 warnings.
The HTML pages are in dart-docs/html.
https://readthedocs.org/api/v2/build/25474391.txt
In addition to adding to the toc:
The converter should be added to 'converter programs' https://docs.dart.ucar.edu/en/latest/observations/obs_converters/README.html#converter-programs
The model should be added to 'supported models'
https://docs.dart.ucar.edu/en/latest/models/README.html
Improved text in the documentation. New model interface and obs converter are now added to the toctree and can be accessed on docs.dart.edu
|
a question/note on the parameter estimation only. This is the docs: "Parameters Estimation only: where only the parameters are constrained using the data. DART will still need to read in the state to construct ensemble covariances and compute innovations. The only difference is that the ensemble increments are only regressed onto the unknown parameters. To achive this goal, you’ll need to set estimate_params = .true. and turn the update status for all state variable to NO_COPY_BACK This ensures that the state will not be updated. The NO_COPY_BACK option is added as the 5th entry in the state table (after the variable name, its associated quantity and its physical bounds) within &model_nml" NO_COPY_BACK means the state is not written out to a restart file. The state is still updated by the assimilation, and inflated. |
There was a problem hiding this comment.
Next batch of review comments for model_mod.
Couple of definite changes:
- namelist items need default values
- param file and state file template files, since you have separate namelist options for turning these on and off, I think it would be better to have a param_template_file and a state_template_file
- model_size is i8 in dart
- for get_close_obs|state this is just passing through to the location mod, so you can use the location mod code directly.
Alphabetical order / style notes:
- MARBL_column before MOM6 https://dart-documentation--726.org.readthedocs.build/en/726/models/README.html
- parameters are CAPITAL LETTERS
- remove commented out print statements
A question on the interpolation, there is no horizontal interpolation so just confirming that that is correct?
The model_time is set for days from 1982. Is this true for all MARBL runs?
There are some other bits and bobs of suggestions for unused variables and old print statements.
models/MARBL_column/model_mod.f90
Outdated
| ! This is a template showing the interfaces required for a model to be compliant | ||
| ! with the DART data assimilation infrastructure. Do not change the arguments | ||
| ! for the public routines. |
There was a problem hiding this comment.
not the template model
Either remove this comment or edit it, e.g.
| ! This is a template showing the interfaces required for a model to be compliant | |
| ! with the DART data assimilation infrastructure. Do not change the arguments | |
| ! for the public routines. | |
| ! This is the MARBL_columun model_mod for the DART data assimilation infrastructure. | |
| ! Do not change the arguments for the public routines. |
models/MARBL_column/model_mod.f90
Outdated
| integer, parameter :: modelvar_table_height = 13 | ||
| integer, parameter :: modelvar_table_width = 5 | ||
| integer, parameter :: modelparams_table_height = 1 | ||
| integer, parameter :: modelparams_table_width = 5 |
There was a problem hiding this comment.
parameters (constants) CAPITAL LETTERS for the names
models/MARBL_column/model_mod.f90
Outdated
| ! Called to do one time initialization of the model. As examples, | ||
| ! might define information about the model size or model timestep. | ||
| ! In models that require pre-computed static data, for instance | ||
| ! spherical harmonic weights, these would also be computed here. |
There was a problem hiding this comment.
| ! Called to do one time initialization of the model. As examples, | |
| ! might define information about the model size or model timestep. | |
| ! In models that require pre-computed static data, for instance | |
| ! spherical harmonic weights, these would also be computed here. | |
| ! Called to do one time initialization of the model. |
| type(time_type) :: mom6_time | ||
| integer :: mom_base_date_in_days, mom_days | ||
|
|
||
| mom_base_date_in_days = 139157 ! 1982 1 1 0 0 |
There was a problem hiding this comment.
Is this date always 1982 1 1 0 0 for MARBL?
For MOM6 the data formate is in days from year 1
There was a problem hiding this comment.
Yes, I believe this the time format for MARBL. I think it has to do with dates related to obs availability. I recall Robin spending some time on this before figuring out the correct format.
models/MARBL_column/model_mod.f90
Outdated
| !print *, "BASIN DEPTH = ",-basin_depth(1,1) | ||
| !print *, "----------------------------------------------------" | ||
| !print *, "computing centers of layers" | ||
| !print *, "----------------------------------------------------" |
There was a problem hiding this comment.
| !print *, "BASIN DEPTH = ",-basin_depth(1,1) | |
| !print *, "----------------------------------------------------" | |
| !print *, "computing centers of layers" | |
| !print *, "----------------------------------------------------" |
models/MARBL_column/model_mod.f90
Outdated
| integer :: param_dom_id ! used to access MARBL internal parameters | ||
| integer :: nfields ! number of fields in the state or parameter vector | ||
| integer :: nz ! the number of vertical layers | ||
| integer :: model_size |
There was a problem hiding this comment.
model_size is an i8 integer in dart.
| integer :: model_size | |
| integer(i8) :: model_size |
| end do | ||
|
|
||
|
|
||
| ! performing the interpolation for each ensemble member individually. |
There was a problem hiding this comment.
This is a question/confirmation for my own understanding. There is no horizontal interpolation, in MARBL_column.
All four gridpoints have identical values for a given level so you only do vertical interpolation, and you just pick point (1,1). You could pick (2,2) and the interpolation would give the same answer.
There was a problem hiding this comment.
Yes, you're right. All four corners are identical columns so technically you can select any corner. This also means that no horizontal interpolation is needed.
models/MARBL_column/model_mod.f90
Outdated
| integer, parameter :: modelparams_table_width = 5 | ||
|
|
||
| ! defining the variables that will be read from the namelist | ||
| character(len=256) :: template_file(2) |
There was a problem hiding this comment.
Because you have this namelist as 2 values, you're requiring that people give two filenames. So even if they
If you only give one file, but estimate_params = .true., you have you get errors like this:
***************** RUNNING TEST 0 ***********************
-- Reading the model_mod namelist and implicitly running static_init_model
**************************************************************
Assimilate_these_obs_types:
BATS_OXYGEN
BATS_ALKALINITY
BATS_INORGANIC_CARBON
BATS_SILICATE
BATS_PHOSPHATE
BATS_NITRATE
Evaluate_these_obs_types:
BATS_ORGANIC_CARBON
BATS_NITROGEN
Use the precomputed Prior Forward Operators for these obs types:
none
ERROR FROM:
source : netcdf_utilities_mod.f90
routine: load_state_variable_info nf90_open
message:
: NetCDF: Malformed URL
message: ...
It would be better to have two namelist options, one for each of state, parameter:
state_template_file = 'state.nc'
parameter_template_file = 'param.nc
In the same way that you hae separate namelist entries for 'model_state_variables' and 'model_parameters'
models/MARBL_column/model_mod.f90
Outdated
| character(len=256) :: ocean_geometry | ||
| real(r8) :: station_location(2) | ||
| integer :: time_step_days | ||
| integer :: time_step_seconds | ||
| logical :: estimate_params |
There was a problem hiding this comment.
all the namelist variables need default values.
otherwise you have the value set to whatever junk is in memory if the namelist value is not set in input.nml
models/MARBL_column/readme.rst
Outdated
| +-------------------------------------+--------------------+------------------------------------------------------------+ | ||
| | Item | Type | Description | | ||
| +=====================================+====================+============================================================+ | ||
| | ``template_file`` | character(len=256) | MARBL restart file including MARBL's prognostic variables | |
There was a problem hiding this comment.
template file is a an array of length 2
see other comments, about making this two namelist options.
Incorporated all suggestions and comments about model-mod. Several comments have been removed, redundant loc routines are omitted, better namelist parsing, deleted unused variables, ...
|
Helen, thanks for the useful comments. I think I addressed all your comments/suggestions and incorporated them in the code. Thanks for clarifying the no-copy-back option. I've edited the text in the documentation to make it more clear. Please let me know if you have any questions. |
|
cool thanks Moha, I'll move on to the obs converter |
| ! by UCAR, "as is", without charge, subject to all terms of use at | ||
| ! http://www.image.ucar.edu/DAReS/DART/DART_download | ||
| ! | ||
| ! $Id$ |
There was a problem hiding this comment.
this is a relic from svn, no need to have it anymore
| ! $Id$ |
| real(r8), parameter :: bats_lon = 360.0_r8 - 64.0_r8 | ||
| real(r8), parameter :: bats_lat = 31.0_r8 |
There was a problem hiding this comment.
CAPITAL LETTERS for parameters
| real(r8), parameter :: bats_lon = 360.0_r8 - 64.0_r8 | |
| real(r8), parameter :: bats_lat = 31.0_r8 | |
| real(r8), parameter :: BATS_LON = 360.0_r8 - 64.0_r8 | |
| real(r8), parameter :: BATS_LAT = 31.0_r8 |
| character(len=256) :: text_input_file, obs_out_dir | ||
| integer :: max_lines | ||
| real(r8) :: obs_err_var_inflation | ||
| logical :: debug |
There was a problem hiding this comment.
need default values for namelist variables
| real(r8) :: qc, obs_err | ||
| real(r8) :: lat, lon, vert, ovalue | ||
|
|
||
| ! the uncertainties corresponding to the observations above |
There was a problem hiding this comment.
not sure what this comment is referencing
| ! adapted for more generic use 11 Mar 2010, nancy collins, ncar/image | ||
| ! | ||
| !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! | ||
| subroutine create_3d_obs(lat, lon, vval, vkind, obsv, otype, oerr, day, sec, qc, obs) |
There was a problem hiding this comment.
Is there reason to have a create_3d_obs in this converter rather than using the create_3d_obs in
obs_utilities_mod?
https://github.com/NCAR/DART/blob/main/observations/obs_converters/utilities/obs_utilities_mod.f90
There was a problem hiding this comment.
I don't think there is any special reason for that, I've removed the ones in the converter and used the tools in obs_utilities_mod and got the same answer. So, I'm cleaning this up as recommended.
|
|
||
| ! unit corrections from BATS to MARBL | ||
| if((OTYPE_ORDERING(otype_index) == BATS_ALKALINITY) .or. (OTYPE_ORDERING(otype_index) == BATS_INORGANIC_CARBON)) then | ||
| ovalue = ovalue*1.026 |
There was a problem hiding this comment.
just a comment, 1.026 is a wild unit conversion.
| os.system("rm -f "+output_txt_path) | ||
| output_txt = open(output_txt_path, "w") |
There was a problem hiding this comment.
this rm -f seems dangerous.
Doesn't open(output_txt_path, "w") overwrite the file if it exists?
| os.system("rm -f "+output_nc_path) | ||
| output_nc = nc.Dataset(output_nc_path, "w") |
| bats_data = open(input_file_path, "r") | ||
| line_number = 0 | ||
|
|
||
| for line in bats_data.readlines(): |
There was a problem hiding this comment.
Not sure how much python reviewing you want on this,
with open is recommended rather than open and close.
You can slice with readlines to skip the first nlines.
with open(input_file_path, 'r') as bats_data:
for line in bats_data.readlines()[first_data_line:]
There was a problem hiding this comment.
I like with open more. The code will be modified accordingly
| @@ -0,0 +1,156 @@ | |||
| import os | |||
| ! QTY_DISSOLVED_ORGANIC_P | ||
| ! QTY_DISSOLVED_INORGANIC_IRON | ||
| ! QTY_SURFACE_CHLOROPHYLL | ||
| ! QTY_LAYER_THICKNESS |
There was a problem hiding this comment.
qty_layer_thickness is in twice in this file
There was a problem hiding this comment.
edit: there are a few duplicates in here:
Duplicate lines containing 'QTY' found:
! QTY_ALKALINITY (appears 2 times)
! QTY_DISSOLVED_INORGANIC_CARBON (appears 2 times)
! QTY_DISSOLVED_INORGANIC_IRON (appears 2 times)
! QTY_DISSOLVED_INORGANIC_SIO3 (appears 2 times)
! QTY_DISSOLVED_ORGANIC_CARBON (appears 2 times)
! QTY_DISSOLVED_ORGANIC_NITROGEN (appears 2 times)
! QTY_DISSOLVED_ORGANIC_P (appears 2 times)
! QTY_DISSOLVED_OXYGEN (appears 2 times)
! QTY_LAYER_THICKNESS (appears 2 times)
! QTY_MESOZOOPLANKTON_CARBON (appears 2 times)
! QTY_MICROZOOPLANKTON_CARBON (appears 2 times)
! QTY_NITRATE_CONCENTRATION (appears 2 times)
! QTY_PHOSPHATE_CONCENTRATION (appears 2 times)
! QTY_PHYTOPLANKTON_BIOMASS (appears 2 times)
! QTY_SURFACE_CHLOROPHYLL (appears 2 times)
|
@hkershaw-brown todo: test netcdf IO change |
Did a cleanup of the obs converter: - Removed comments - Removed unused variables - Improved style - Better use of DART's modules - Improved initialization and functions - Ran and tested the converter after the changes were made
|
@hkershaw-brown I incorporated all of your comments and suggestions from the obs converter. |
|
@mgharamti do you have a wrfhydro case (or other model) you can run with this branch? |
|
@hkershaw-brown this is to confirm that the netcdf changes are good. I've ran filter with different |
|
nice thanks @mgharamti |
|
@mgharamti this is good to go, but let's release it next week (paranoia for netcdf changes on a Friday) |
|
sounds good to me. thanks for your help reviewing this. |
Description:
A new model interface to the biogeochemistry model MARBL. The work done here is contributed by Robin Armstrong. Thanks Robin!
Fixes issue
This address all comments, suggestions, and questions raised on the original PR MARBL-DART #701. It also fixes issue #614
Types of changes
Documentation changes needed?
Tests
Ran several DART program to assess the behavior:
model_mod_check,filter, andbats_to_obsMy tests are here:
/glade/work/gharamti/DART/bgc_work/models/MARBL_column/work/glade/work/gharamti/DART/bgc_work/observations/obs_converters/BATSChecklist for merging
Checklist for release
Testing Datasets