mpas_atm refactor to use state structure module #780

hkershaw-brown · 2024-12-09T14:27:18Z

Description:

Refactor mpas_atm model_mod to use state_structure, remove progvar to vector.

Fixes issue

Various mpas_atm issues, notes in #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

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Documentation changes needed?

My change requires a change to the documentation.
- I have updated the documentation accordingly.

Tests

MPAS model_mod_check and filter

Checklist for merging

Updated changelog entry
Documentation updated
Update conf.py

Checklist for release

Merge into main
Create release from the main branch with appropriate tag
Delete feature-branch

Testing Datasets

Dataset needed for testing available upon request
Dataset download instructions included
No dataset needed

… progvar

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.

see #768

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

hkershaw-brown · 2024-12-09T14:29:40Z

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

hkershaw-brown · 2025-03-19T13:45:23Z

@mjs2369 Jeff's not looking at mpas dart code (my bad I though he was working on this), so I'm adding you as a reviewer.
It is a decent sized pull request, so take a look. Let me know if you want to go over the code and changes, and the original code.

jlaucar · 2025-03-21T20:36:39Z

Helen, I'm going to go ahead and do my best to review this, too. Sorry for confusion back and forth. I'm very glad that Marlee will be doing a review which will no doubt be better than mine. Is this something that would be worth a code review with the three of us sometime? Have a good weekend, Jeff

…

On Wed, Mar 19, 2025 at 7:45 AM Helen Kershaw ***@***.***> wrote: @mjs2369 <https://github.com/mjs2369> Jeff's not looking at mpas dart code (my bad I though he was working on this), so I'm adding you as a reviewer. It is a decent sized pull request, so take a look. Let me know if you want to go over the code and changes, and the original code. — Reply to this email directly, view it on GitHub <#780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANDHUIUHJMD6ECLLHJYVWQD2VFYIVAVCNFSM6AAAAABTI6UE3OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMZWG4YDMMRQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: hkershaw-brown]*hkershaw-brown* left a comment (NCAR/DART#780) <#780 (comment)> @mjs2369 <https://github.com/mjs2369> Jeff's not looking at mpas dart code (my bad I though he was working on this), so I'm adding you as a reviewer. It is a decent sized pull request, so take a look. Let me know if you want to go over the code and changes, and the original code. — Reply to this email directly, view it on GitHub <#780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANDHUIUHJMD6ECLLHJYVWQD2VFYIVAVCNFSM6AAAAABTI6UE3OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMZWG4YDMMRQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

models/mpas_atm/model_mod.f90

find_vert_level: - use var_id for vertisheight, maybe on nVertLevelsP1, nVertLevels - Use var_id for vertislevel, maybe on nVertLevelsP1, nVertLevels - use nVertLevels for vertispressure - use nVertlevels if obs_qty is not in state on_edge: - If qty is not in state on_edge = .false. Also too_high magic number 81 replaced with too high parameter

models/mpas_atm/MPAS_model_modules/get_coeff_mod.f90

Removed save hanging out, module variables are saved.

models/mpas_atm/update_mpas_states.f90

jlaucar · 2025-04-07T19:45:42Z

Line 899 in model_mod.f90/qty_ok_to_interpolate
Would it be clearer to roll all the cases that have identical actions into a single case?

Line 167: Why a separate public statement for these three things?

HK fixed.

mjs2369 · 2025-04-22T15:23:42Z

Few more comment fixes:

DART/models/mpas_atm/model_mod.f90

Lines 4330 to 4331 in 381b40e

    
           ! FIXME: 
        
           !        we can hard-code R3 here since it comes from the (3d) x/y/z cartesian coordinate.

R3 is already hardcoded here, think we can remove these lines - maybe just keep the part "comes from the (3d) x/y/z cartesian coordinate" and move it next to where R3 is defined (line 4341 and 4310)

DART/models/mpas_atm/model_mod.f90

Lines 1804 to 1805 in 381b40e

    
           string1 = 'WARNING: There is a possibility you need to increase ''MAX_STATE_VARIABLES''' 
        
           write(string2,'(''WARNING: you have specified at least '',i4,'' perhaps more.'')')ngood

A bit confusing in the wording, is like maybe you should increase MAX_STATE_VARIABLES, but it's not really clear that's what the comment is suggesting. Also, do we want to recommend people change MAX_STATE_VARIABLES? Maybe we just make this a bit bigger now

mjs2369 · 2025-04-22T15:42:55Z

Just to clarify:

DART/models/mpas_atm/model_mod.f90

Lines 902 to 917 in 381b40e

    
           select case (qty) 
        
           case (QTY_TEMPERATURE, QTY_2M_TEMPERATURE) 
        
              qty_ok = .true. 
        
           case (QTY_SURFACE_ELEVATION, QTY_GEOPOTENTIAL_HEIGHT) 
        
              qty_ok = .true. 
        
           case (QTY_PRESSURE)   ! surface pressure should be in the state !HK @todo check "should" 
        
              qty_ok = .true. 
        
           case (QTY_SKIN_TEMPERATURE, QTY_SURFACE_TYPE, QTY_CLOUD_FRACTION)    
        
              qty_ok = .true. 
        
           case (QTY_SPECIFIC_HUMIDITY, QTY_2M_SPECIFIC_HUMIDITY) 
        
              qty_ok = .true. 
        
           case (QTY_U_WIND_COMPONENT, QTY_V_WIND_COMPONENT) 
        
              if (get_varid_from_kind(anl_domid, QTY_EDGE_NORMAL_SPEED) > 0 .and. use_u_for_wind) qty_ok = .true. 
        
           case default 
        
              qty_ok = .false. 
        
           end select

Is this function (qty_ok_to_interpolate) specifying the QTYs that we can interpolate even if they are not in the state vector?

hkershaw-brown · 2025-04-23T21:02:07Z

Notes meeting April 23rd 2025:

documentation for wind options is needed, what you can select and what happens.

question: has_edge_u .and. use_u_for_wind for the condition to call compute_u_with_rbf vs. call compute_scalar_with_barycentric if one or both is .false.

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.

hkershaw-brown · 2025-05-07T19:29:21Z

Line 899 in model_mod.f90/qty_ok_to_interpolate Would it be clearer to roll all the cases that have identical actions into a single case?

Line 167: Why a separate public statement for these three things?

rolled case together & joined public statements in 52b87fc

hkershaw-brown · 2025-05-07T19:30:30Z

Just to clarify:

DART/models/mpas_atm/model_mod.f90

Lines 902 to 917 in 381b40e

select case (qty)

case (QTY_TEMPERATURE, QTY_2M_TEMPERATURE)

qty_ok = .true.

case (QTY_SURFACE_ELEVATION, QTY_GEOPOTENTIAL_HEIGHT)

qty_ok = .true.

case (QTY_PRESSURE) ! surface pressure should be in the state !HK @todo check "should"

qty_ok = .true.

case (QTY_SKIN_TEMPERATURE, QTY_SURFACE_TYPE, QTY_CLOUD_FRACTION)

qty_ok = .true.

case (QTY_SPECIFIC_HUMIDITY, QTY_2M_SPECIFIC_HUMIDITY)

qty_ok = .true.

case (QTY_U_WIND_COMPONENT, QTY_V_WIND_COMPONENT)

if (get_varid_from_kind(anl_domid, QTY_EDGE_NORMAL_SPEED) > 0 .and. use_u_for_wind) qty_ok = .true.

case default

qty_ok = .false.

end select

Is this function (qty_ok_to_interpolate) specifying the QTYs that we can interpolate even if they are not in the state vector?

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.

Copilot

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/statevector logic for state_structure_mod APIs and nc_get_variable/nc_put_variable.
Introduce branching logic to handle reconstructed vs. direct wind updates in both update_mpas_states.f90 and update_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 private without any public declarations means none of the routines are exported. You need to explicitly mark the intended subroutines (mpas_*, etc.) as public.

     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',

Copilot · 2025-06-09T16:51:21Z

models/mpas_atm/update_mpas_states.f90

+        call nc_put_variable(ncBckID, 'u', variable)
+        call nc_put_variable(ncBckID, 'uReconstructZonal', variable)
+        call nc_put_variable(ncBckID, 'uReconstructMeridional', variable)


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).

Suggested change

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)

Copilot · 2025-06-09T16:51:21Z

models/mpas_atm/update_bc.f90

+      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


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).

Suggested change

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

Copilot · 2025-06-09T16:51:22Z

models/mpas_atm/update_mpas_states.f90

@@ -14,83 +14,77 @@ program update_mpas_states
 !         The update_mpas_states_nml namelist defines the input and output file
 !         name lists for all ensemble members.
 !         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


[nitpick] Minor grammar: remove "the" before "another". For example: "variables that are not wind are copied from one file to another."

Suggested change

! 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

hkershaw-brown added 23 commits October 11, 2024 08:40

first pass through removing uneeded code

fdd2518

model_mod_check

c8c9b0e

get_state_meta_data - bitwise with main

563ed3c

model_interpolate error codes

10d63eb

compute_scalar_with_barycentric with state stucture calls rather than…

4a82fb7

… progvar

state variables bounds

3cccd4a

Note still have clamp or fail being read but not used #624

model_interpolate

9aa40b0

Removed state_vector_to_boundary_file and update_wind_components so I can compile with debugging flags

get_close and convert_vert for obs and state

0892248

Merge branch 'main' into mpas-refactor

a788026

uncommented find_surrounding_edges

8d718c4

Is the rbf code depricated #753 (comment)

compute_pressure_at_loc

2fc8672

is_edgedata_in_state_vector checks state vector for edge dimensions.

6db8018

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.

chore: removed unused routine query_vert_localization_coord

268f5af

#753 (comment) removed done todo comment

chore: remove unsused variables from get_state_meta_data

14e6b4a

Removed more unused code

ab1d1aa

statevector_to_boundary_file and statevector_to_analysis file need to be done.

Merge branch 'main' into mpas-refactor

061acab

note on 2M, 10M qtys

9be3b78

see #768

feat: model_interpolate for qty_vertical_velocity

ea7044f

todo: the pressure on edges

fix: nc can be more than 3 for rbf calculations

15c654c

set ids array to length nc rather than 3

reject obs to far from surface

f02f271

Q. Are there any surface obs that are not required for RTTOV?

reject obs above a user specified pressure level

3716ed2

Things that cannot be negative

8a6d6dd

QTY_CLOUD_FRACTION is also limited to 1

Merge branch 'main' into mpas-refactor

4f2df10

hkershaw-brown mentioned this pull request Dec 9, 2024

mpas code from MPAS-Model in DART repo is missing the MPAS-Model is missing the LICENCE #781

Open

hkershaw-brown added the mpas Model for Prediction Across Scales (MPAS) label Dec 11, 2024

hkershaw-brown added 4 commits December 24, 2024 13:27

chore: add LICENSCE for code from MPAS-Model

f8e40b3

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

remove out-of-date revision, revdate

40eda41

update_mpas_states - copying variables from one file to another

78ba958

todo: wind, domain_id see #753 (comment) for various notes on update_mpas_states

chore: remove read_2d_from_nc_file

4404e17

have netcdf utitilies module for this

hkershaw-brown requested a review from mjs2369 March 19, 2025 14:09

hkershaw-brown commented Mar 26, 2025

View reviewed changes

models/mpas_atm/model_mod.f90 Outdated Show resolved Hide resolved

hkershaw-brown and others added 2 commits March 31, 2025 09:17

Merge branch 'main' into mpas-refactor

223bfd8

mjs2369 reviewed Apr 1, 2025

View reviewed changes

models/mpas_atm/MPAS_model_modules/get_coeff_mod.f90 Outdated Show resolved Hide resolved

remove svn revision, revdate

381b40e

Removed save hanging out, module variables are saved.

mjs2369 reviewed Apr 3, 2025

View reviewed changes

models/mpas_atm/update_mpas_states.f90 Outdated Show resolved Hide resolved

Merge branch 'main' into mpas-refactor

13ee8e5

hkershaw-brown and others added 4 commits May 6, 2025 10:01

Merge branch 'main' into mpas-refactor

18b2134

fix: non-zero error for all ensemble members if one fails in find_ver…

069bf87

…t_level Previously returning early with 'success' for some ensemble members, even though bailing from the routine.

Merge branch 'main' into mpas-refactor

bfade98

comments: tidy, join case statements that have same result.

52b87fc

hkershaw-brown added 4 commits May 13, 2025 14:58

fix: update_mpas_states to get size of u from the file when not in th…

570c7a8

…e state Note this commit has netcdf fix for #874

remove unused variable- increments

eac0487

doc: namelist documumentation

b34549b

match namelist with options in code removed strange space characters

hkershaw-brown force-pushed the mpas-refactor branch from ce322d9 to aa5435e Compare May 23, 2025 19:10

hkershaw-brown mentioned this pull request Jun 4, 2025

clamp or fail - mpas and wrf model_mod have this, it is ignored by dart #624

Open

Merge branch 'main' into mpas-refactor

2f34fed

hkershaw-brown requested a review from Copilot June 9, 2025 16:48

Copilot AI reviewed Jun 9, 2025

View reviewed changes

hkershaw-brown mentioned this pull request Jun 9, 2025

feat req: Memory scalable static data in DART - both static across the ensemble and per-ensemble member static data #744

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

mpas_atm refactor to use state structure module #780

mpas_atm refactor to use state structure module #780

Uh oh!

hkershaw-brown commented Dec 9, 2024 •

edited

Loading

Uh oh!

hkershaw-brown commented Dec 9, 2024

Uh oh!

hkershaw-brown commented Mar 19, 2025

Uh oh!

jlaucar commented Mar 21, 2025 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jlaucar commented Apr 7, 2025 •

edited by hkershaw-brown

Loading

Uh oh!

mjs2369 commented Apr 22, 2025

Uh oh!

mjs2369 commented Apr 22, 2025

Uh oh!

hkershaw-brown commented Apr 23, 2025 •

edited

Loading

Uh oh!

hkershaw-brown commented May 7, 2025

Uh oh!

hkershaw-brown commented May 7, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 9, 2025

Uh oh!

Copilot AI Jun 9, 2025

Uh oh!

Copilot AI Jun 9, 2025

Uh oh!

Uh oh!

		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

	! 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

mpas_atm refactor to use state structure module #780

Are you sure you want to change the base?

mpas_atm refactor to use state structure module #780

Uh oh!

Conversation

hkershaw-brown commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Fixes issue

Types of changes

Documentation changes needed?

Tests

Checklist for merging

Checklist for release

Testing Datasets

Uh oh!

hkershaw-brown commented Dec 9, 2024

Uh oh!

hkershaw-brown commented Mar 19, 2025

Uh oh!

jlaucar commented Mar 21, 2025 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jlaucar commented Apr 7, 2025 • edited by hkershaw-brown Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjs2369 commented Apr 22, 2025

Uh oh!

mjs2369 commented Apr 22, 2025

Uh oh!

hkershaw-brown commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkershaw-brown commented May 7, 2025

Uh oh!

hkershaw-brown commented May 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hkershaw-brown commented Dec 9, 2024 •

edited

Loading

jlaucar commented Apr 7, 2025 •

edited by hkershaw-brown

Loading

hkershaw-brown commented Apr 23, 2025 •

edited

Loading