mpas_atm refactor to use state structure module#780
mpas_atm refactor to use state structure module#780hkershaw-brown wants to merge 62 commits intomainfrom
Conversation
Note still have clamp or fail being read but not used #624
Removed state_vector_to_boundary_file and update_wind_components so I can compile with debugging flags
Is the rbf code depricated #753 (comment)
see comment #753 (comment) Remove not used variables: add_static_data_to_diags - not used ew_boundary_type, ns_boundary_type -not used Shortened some very verbose comments.
#753 (comment) removed done todo comment
statevector_to_boundary_file and statevector_to_analysis file need to be done.
todo: the pressure on edges
set ids array to length nc rather than 3
Q. Are there any surface obs that are not required for RTTOV?
QTY_CLOUD_FRACTION is also limited to 1
|
mpas code from MPAS-Model in DART repo is missing the MPAS-Model is missing the LICENCE. |
moved MPAS-model code to a directory with the LICENCE for the code from https://github.com/MPAS-Dev/MPAS-Model/blob/master/LICENSE fixes #781
todo: wind, domain_id see #753 (comment) for various notes on update_mpas_states
have netcdf utitilies module for this
|
Just to clarify: DART/models/mpas_atm/model_mod.f90 Lines 902 to 917 in 381b40e Is this function (qty_ok_to_interpolate) specifying the QTYs that we can interpolate even if they are not in the state vector? |
|
Notes meeting April 23rd 2025: documentation for wind options is needed, what you can select and what happens.
Jla: error on "deprecated" option RBF since there is a bug in the main version: #861 edit: HK depricated use_u_for_wind in aa5435e |
…t_level Previously returning early with 'success' for some ensemble members, even though bailing from the routine.
rolled case together & joined public statements in 52b87fc |
Yup you got it. Some are static, some you need to have other variables. |
…e state Note this commit has netcdf fix for #874
match namelist with options in code removed strange space characters
On advice from jla: error if a user has selected use_u_for_wind as there is a bug in Manhattan, where all ensemble members are set to the last ensemble member value in the rbf calculation #861 see #780 (comment) Documentation for mpas regristry changes needed for DART Regional wind updates namelist options in update_bc - not sure whey they do not use the same model_nml options, so not added to docs in this commit.
hkershaw-brown
force-pushed
the
mpas-refactor
branch
from
ce322d9 to
aa5435e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MPAS ATM model_mod utilities to use the state_structure module for variable access, replaces the old progvar-to-vector approach, and cleans up netCDF handling across initialization, state updating, and boundary condition routines.
- Swap out
read_variables/statevectorlogic forstate_structure_modAPIs andnc_get_variable/nc_put_variable. - Introduce branching logic to handle reconstructed vs. direct wind updates in both
update_mpas_states.f90andupdate_bc.f90. - Remove legacy namelist flags and model utilities, update input defaults in
input.nml.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| models/mpas_atm/work/input.nml | Updated default restart filename and cleaned up obsolete flags |
| models/mpas_atm/update_mpas_states.f90 | Refactored state updates to use state_structure_mod; wind logic |
| models/mpas_atm/update_bc.f90 | Refactored boundary updates to use state_structure_mod APIs |
| models/mpas_atm/mpas_dart_obs_preprocess.f90 | Removed unused grid check; cleaned up metadata parameters |
| models/mpas_atm/MPAS_model_modules/get_reconstruct_mod.f90 | Added module privacy controls |
| models/mpas_atm/MPAS_model_modules/get_geometry_mod.f90 | Added private but omitted necessary public exports |
| models/mpas_atm/MPAS_model_modules/get_coeff_mod.f90 | Streamlined version metadata and error handler calls |
| models/mpas_atm/MPAS_model_modules/LICENSE | Added license file for MPAS module code |
| assimilation_code/modules/observations/obs_kind_mod.rst | Minor RST formatting |
Comments suppressed due to low confidence (3)
models/mpas_atm/MPAS_model_modules/get_geometry_mod.f90:6
- Declaring the module as
privatewithout anypublicdeclarations means none of the routines are exported. You need to explicitly mark the intended subroutines (mpas_*, etc.) aspublic.
private
models/mpas_atm/update_mpas_states.f90:116
- The logical OR operator is malformed here (
'.or.'appears attached to the string literal). It should read.or.between each equality check, for example:
if (name == 'uReconstructZonal' .or. &
name == 'uReconstructMeridional' .or. &
name == 'u') cycle varloop
get_variable_name(dom_id,i) == 'uReconstructMeridional'.or. &
models/mpas_atm/work/input.nml:213
- Missing comma between the state variable name and its quantity code. It should be
't2m', 'QTY_2M_TEMPERATURE',.
't2m' 'QTY_2M_TEMPERATURE',
| call nc_put_variable(ncBckID, 'u', variable) | ||
| call nc_put_variable(ncBckID, 'uReconstructZonal', variable) | ||
| call nc_put_variable(ncBckID, 'uReconstructMeridional', variable) |
There was a problem hiding this comment.
In this branch you intended to write the reconstructed wind array u, but you are passing variable (from the non-wind loop). Replace variable with u (and similarly for the subsequent calls writing zonal/meridional arrays).
| call nc_put_variable(ncBckID, 'u', variable) | |
| call nc_put_variable(ncBckID, 'uReconstructZonal', variable) | |
| call nc_put_variable(ncBckID, 'uReconstructMeridional', variable) | |
| call nc_put_variable(ncBckID, 'u', u) | |
| call nc_put_variable(ncBckID, 'uReconstructZonal', ucell_dart) | |
| call nc_put_variable(ncBckID, 'uReconstructMeridional', vcell_dart) |
| call nc_get_variable(ncBdyID, 'lbc_ur', lbc_vcell) ! already blended in VARLOOP | ||
| call nc_get_variable(ncBdyID, 'lbc_vr', lbc_ucell) ! already blended in VALOOP |
There was a problem hiding this comment.
The data from 'lbc_ur' (zonal reconstruct) is being read into lbc_vcell (meridional array) by mistake. These two should be swapped (lbc_ur→lbc_ucell, lbc_vr→lbc_vcell).
| call nc_get_variable(ncBdyID, 'lbc_ur', lbc_vcell) ! already blended in VARLOOP | |
| call nc_get_variable(ncBdyID, 'lbc_vr', lbc_ucell) ! already blended in VALOOP | |
| call nc_get_variable(ncBdyID, 'lbc_ur', lbc_ucell) ! already blended in VARLOOP | |
| call nc_get_variable(ncBdyID, 'lbc_vr', lbc_vcell) ! already blended in VALOOP |
| ! The input list should be matched with output_state_file_list in &filter_nml. | ||
| ! | ||
| ! | ||
| ! variables that are not wind are copied from one file to the another |
There was a problem hiding this comment.
[nitpick] Minor grammar: remove "the" before "another". For example: "variables that are not wind are copied from one file to another."
| ! variables that are not wind are copied from one file to the another | |
| ! variables that are not wind are copied from one file to another |
|
still to do regional. /glade/derecho/scratch/hkershaw/DART/MPAS_test_cases_april_2025 |
|
Closing this pull request for now since it is coming up on 11 months. Will reopen if/when mpas users are available. |
|
ditto
|
Description:
Refactor mpas_atm model_mod to use state_structure, remove progvar to vector.
Fixes issue
Various mpas_atm issues, notes in #753
fixes #753
fixes #523,
fixes mpas part of #389
fixes #781
#624, #768
note on subroutines, functions in mpas_atm model_mod refactor vs removed vs unchanged
Types of changes
Documentation changes needed?
Tests
MPAS model_mod_check and filter
Checklist for merging
Checklist for release
Testing Datasets