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

Add the mean EnKF soil increment to the deterministic member #3295

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from

Conversation

ClaraDraper-NOAA
Copy link
Contributor

@ClaraDraper-NOAA ClaraDraper-NOAA commented Feb 4, 2025

Description

Add mean EnKF soil increment to the deterministic member. Includes new regriding script and executable (in GDASApp) to perform masked bi-linear interpolation of the soil increments.

Resolves 2773

Type of change

  • Bug fix (fixes something broken)
  • [ x] New feature (adds functionality)
  • Maintenance (code refactor, clean-up, new CI test, etc.)

Change characteristics

  • Is this a breaking change (a change in existing functionality)? NO
  • Does this change require a documentation update? NO
  • Does this change require an update to any of the following submodules?
    • [ YES] UFS-utils

OUTDATED:
Exercizing the new soil analysis feature requires updating UFS_UTILS to at least commit e718003

UPDATE Feb 5:
After responding to reviews, requires updating to UFS_UTILS PR 1022

How has this been tested?

Tested on hera:
C96_atm3DVar, C96C48_hybatmaerosnowDA, and the default test ran succesfully.
Confirmed new functionality works as expected.
Confirmed that the commits do not change output from cycling the DA if the the new feature is not selected.

Checklist

  • [ x] Any dependent changes have been merged and published
  • [x ] My code follows the style guidelines of this project
  • [ x] I have performed a self-review of my own code
  • [x ] I have commented my code, particularly in hard-to-understand areas
  • [x ] I have documented my code, including function, input, and output descriptions
  • [ x] My changes generate no new warnings
  • [ x] New and existing tests pass with my changes
  • This change is covered by an existing CI test or a new one has been added
  • [x ] Any new scripts have been added to the .github/CODEOWNERS file with owners
  • [x ] I have made corresponding changes to the system documentation if necessary

NOTE:

  • Exercizing the new feature requires some additional fix files, currently at $TMP_FIX_FILES in ush/regrid_gsiSfcIncr_to_tile.sh . These will need to be added to a fix directory, if this PR is accepted as is (note: these files could also be generated on the fly)
  • Exercizing the new feature requires jobs/rocoto/sfcanl.sh and sfc.sh to load the ufsda modules in place of the fv3gfs modules. My understanding is that the modules will eventually be unified, but for now I've added comments to the relevant scripts noting this.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShellCheck found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first pass

env/GAEAC5.env Outdated Show resolved Hide resolved
@@ -24,6 +24,9 @@ YMD=${PDY} HH=${cyc} declare_from_tmpl -rx \
YMD=${PDY} HH=${cyc} declare_from_tmpl -rx \
COMOUT_ATMOS_RESTART:COM_ATMOS_RESTART_TMPL

RUN="enkfgdas" MEMDIR="ensstat" YMD=${PDY} HH=${cyc} declare_from_tmpl -rx \
COM_ATMOS_ENKF_ANALYSIS_STAT:COM_ATMOS_ANALYSIS_TMPL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COM needs to be explicit if it is COMIN or COMOUT now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to COMIN.

parm/config/gfs/config.sfcanl Outdated Show resolved Hide resolved
@@ -225,6 +239,9 @@ if [ $DOSFCANL_ENKF = "YES" ]; then
RUN="${GDUMP_ENS}" MEMDIR=${gmemchar} YMD=${gPDY} HH=${gcyc} declare_from_tmpl \
COMIN_ATMOS_RESTART_MEM_PREV:COM_ATMOS_RESTART_TMPL

MEMDIR=${memchar} YMD=${PDY} HH=${cyc} declare_from_tmpl \
COM_ATMOS_ANALYSIS_MEM:COM_ATMOS_ANALYSIS_TMPL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same COM comment as before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to COMIN

scripts/exgdas_enkf_sfc.sh Outdated Show resolved Hide resolved
scripts/exglobal_atmos_sfcanl.sh Outdated Show resolved Hide resolved
memchar="mem${cmem}"

MEMDIR=${memchar} YMD=${PDY} HH=${cyc} declare_from_tmpl \
COM_ATMOS_ANALYSIS_MEM:COM_ATMOS_ANALYSIS_TMPL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more COM related comments in this file

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes looks good. seems like some checks are failing. I tried to recommend a couple updates that might help.

ush/regrid_gsiSfcIncr_to_tile.sh Outdated Show resolved Hide resolved
ush/regrid_gsiSfcIncr_to_tile.sh Outdated Show resolved Hide resolved
@ClaraDraper-NOAA
Copy link
Contributor Author

Can someone please point me to the reason for the failure? I see warnings for code that I haven't changed. Am I meant to fix these?

Comment on lines 16 to 19
export DO_LNDINC=".true."
export DO_SOI_INC=".true."
export GCYCLE_INTERP_LNDINC=".false."
export LSOIL_INCR=2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nitpicky comment - and is a case in the global-workflow in general - variables do not need to be 3 characters long e.g. DO_SOI_INC -> DO_SOIL_INC or DO_SOILINC to be (in)consistent with DO_LNDINC or something. Even INC is sometimes INCR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not nitpicky at all! Or at least, it has been annoying me too. Also the mix of YES/NO, and true/false. Some of it is coming from the namelists used in submodules. I'll at least establish consistent variable names for most of the workflow, then try to clean it up over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm preparing to fix this, with a second PR to UFS_UTILS, resolving issue 1021

@DavidHuber-NOAA
Copy link
Contributor

@ClaraDraper-NOAA If we still see errors after fixing the issue in ush/regrid_gsiSfcIncr_to_tile.sh, I can submit a PR to your branch to fix the remaining issues in scripts/exgdas_enkf_sfc.sh.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a pass through on this PR and left a few comments and suggestions. Please let me know if I can help clarify or assist.

scripts/exgdas_enkf_sfc.sh Outdated Show resolved Hide resolved
scripts/exglobal_atmos_sfcanl.sh Outdated Show resolved Hide resolved
scripts/exglobal_atmos_sfcanl.sh Outdated Show resolved Hide resolved
scripts/exglobal_atmos_sfcanl.sh Outdated Show resolved Hide resolved
scripts/exglobal_atmos_sfcanl.sh Outdated Show resolved Hide resolved
ush/regrid_gsiSfcIncr_to_tile.sh Outdated Show resolved Hide resolved
ush/regrid_gsiSfcIncr_to_tile.sh Outdated Show resolved Hide resolved
ush/regrid_gsiSfcIncr_to_tile.sh Show resolved Hide resolved
ush/regrid_gsiSfcIncr_to_tile.sh Outdated Show resolved Hide resolved
.github/CODEOWNERS Show resolved Hide resolved
@ClaraDraper-NOAA
Copy link
Contributor Author

@ClaraDraper-NOAA If we still see errors after fixing the issue in ush/regrid_gsiSfcIncr_to_tile.sh, I can submit a PR to your branch to fix the remaining issues in scripts/exgdas_enkf_sfc.sh.

I can fix exgdas_enfkf_sfs.sh, I just wasn't sure if this was even the problem since they're warnings.

@@ -24,6 +24,9 @@ YMD=${PDY} HH=${cyc} declare_from_tmpl -rx \
YMD=${PDY} HH=${cyc} declare_from_tmpl -rx \
COMOUT_ATMOS_RESTART:COM_ATMOS_RESTART_TMPL

RUN="enkfgdas" MEMDIR="ensstat" YMD=${PDY} HH=${cyc} declare_from_tmpl -rx \
COMIN_ATMOS_ENKF_ANALYSIS_STAT:COM_ATMOS_ANALYSIS_TMPL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking for the current ensemble update analysis stat directory correct? Does having RUN="enkgdas" cause a failure during the gfs cycle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've turned off DO_GSISOILDA for the gfs runs.

@ClaraDraper-NOAA
Copy link
Contributor Author

All:

  1. I think I've addressed everything. I resolved any comments where I thought the change was obvious. Given the large number of changes, and my propensity for typos, I'm going to repeat the tests to make sure I didn't mess anything up. I'll comment again once that's done.

  2. I've also found a bug I need to fix.

  3. I've prepared a PR in UFS_UTILS to tidy up the variable names, as I think this is a good time to do that. I'll edit the first comment above to note that.

Copy link
Contributor

@CoryMartin-NOAA CoryMartin-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last comment, otherwise I think the changes look good (have not tested).

We should turn this on in one or more of the CI tests

Comment on lines +16 to +19
_RUN=${RUN:-"gfs"}
_RUN=${RUN/enkf/}

if [[ "${_RUN}" == "gfs" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_RUN=${RUN:-"gfs"}
_RUN=${RUN/enkf/}
if [[ "${_RUN}" == "gfs" ]]; then
if [[ "${RUN/enkf/}" == "gfs" ]]; then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should always be a RUN I think

@ClaraDraper-NOAA
Copy link
Contributor Author

last comment, otherwise I think the changes look good (have not tested).

We should turn this on in one or more of the CI tests

I've added a CI test.

@ClaraDraper-NOAA
Copy link
Contributor Author

All - I have one change I need to make tomorrow (staging the orog fix files), which will require a fortran change. Otherwise, I think everything else is done. Thanks for all your input!

@RussTreadon-NOAA RussTreadon-NOAA modified the milestone: GFS v17 Feb 8, 2025
export GCYCLE_DO_SOILINCR=".true."
export GCYCLE_INTERP_LANDINCR=".false."
export LSOIL_INCR=2
export REGRID_EXEC=${REGRID_EXEC:-"${HOMEgfs}/sorc/gdas.cd/build/bin/regridStates.x"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export REGRID_EXEC=${REGRID_EXEC:-"${HOMEgfs}/sorc/gdas.cd/build/bin/regridStates.x"}
export REGRID_EXEC="${HOMEgfs}/sorc/gdas.cd/build/bin/regridStates.x"

No need for a pass through here since it is configured here first.

Comment on lines +24 to +27
_RUN=${RUN:-"gfs"}
_RUN=${RUN/enkf/}

if [[ "${_RUN}" == "gfs" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_RUN=${RUN:-"gfs"}
_RUN=${RUN/enkf/}
if [[ "${_RUN}" == "gfs" ]]; then
if [[ "${RUN/enkf}" == "gfs" ]]; then

Don't really need _RUN.

export CASE_OUT=${CASE_ENS}
export OCNRES_OUT=${OCNRES}
export NMEM_REGRID=${NMEM_ENS}
if [ $DOIAU = "YES" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if [ $DOIAU = "YES" ]; then
if [[ "${DOIAU}" == "YES" ]]; then

export LFHR=6 # PDYcyc
fi

$REGRIDSH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$REGRIDSH
"${REGRIDSH}"

done # ensembles

done

fi

if [ $DOSFCANL_ENKF = "YES" ]; then
if [[ $DOSFCANL_ENKF = "YES" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if [[ $DOSFCANL_ENKF = "YES" ]]; then
if [[ "${DOSFCANL_ENKF}" == "YES" ]]; then

ush/regrid_gsiSfcIncr_to_tile.sh Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add soil increments to deterministic member
6 participants