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

Various wave updates: Reformat WW3 inputs; enable PIO for wave restarts; enable WW3 cycling; synchronize UFS restart writes; add uglo_15km WW3 grid #3190

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

Conversation

JessicaMeixner-NOAA
Copy link
Contributor

@JessicaMeixner-NOAA JessicaMeixner-NOAA commented Dec 20, 2024

Description

This PR adds the following:

Note this PR requires the following:

  • update to fix files to add uglo_15km
  • staging ICs for high resolution test case for uglo_15km

Co-author: @sbanihash

Related Issues:

Type of change

  • Bug fix (fixes something broken)
  • New feature (adds functionality)

Change characteristics

How has this been tested?

Tests were done by adding waves to C48mx500 3DVar tests for cycling.
High resolution tests cases were also added.

This likely needs a new round of testing before CI and after staged ICs & new fix files are added.

Checklist

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

JessicaMeixner-NOAA and others added 30 commits July 15, 2024 16:55
@DavidHuber-NOAA DavidHuber-NOAA changed the title Various wave updates Reformat WW3 inputs; enable PIO for wave restarts; enable WW3 cycling; synchronize UFS restart writes; add uglo_15km WW3 grid Dec 23, 2024
JessicaMeixner-NOAA and others added 18 commits December 30, 2024 16:14
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
Co-authored-by: David Huber <69919478+DavidHuber-NOAA@users.noreply.github.com>
@JessicaMeixner-NOAA JessicaMeixner-NOAA changed the title Reformat WW3 inputs; enable PIO for wave restarts; enable WW3 cycling; synchronize UFS restart writes; add uglo_15km WW3 grid Various wave updates: Reformat WW3 inputs; enable PIO for wave restarts; enable WW3 cycling; synchronize UFS restart writes; add uglo_15km WW3 grid Dec 30, 2024
@@ -25,6 +25,7 @@ local SHOUR=${model_start_date:8:2}
local FHROT=${IAU_FHROT:-0}
local DT_ATMOS=${DELTIM}
local RESTART_INTERVAL="${FV3_RESTART_FH[*]}"
local RESTART_FH="${CMEPS_RESTART_FH:-" "}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure introducing a CMEPS variable dependency in FV3 would be clean, since that means there is a dependency on a coupled model variable for an atm-only configuration. Need to think a bit on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RESART_FH variable is not used by FV3, however it is in the model_configure file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also - please note this is what is being done in this PR: ufs-community/ufs-weather-model#2419 so if you want changes we'll potentially need changes to ufs-weather-model

Copy link
Contributor

Choose a reason for hiding this comment

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

If the FV3 does not use this variable, can we just set this as:
local RESTART_FH=" "

Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes the restarts are written according to local RESTART_INTERVAL="${FV3_RESTART_FH[*]}", correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I misunderstood this comment that said the FV3 does not use this variable. Does the model_configure also now use this variable to specify restart write times for all model components? (In the past, model_configure was fv3atm specific)
How does this relate to RESTART_INTERVAL that is currently specifying the FV3 restart times? Does RESTART_FH override RESTART_INTERVAL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @NickSzapiro-NOAA in an email:

3 restart triggers:
restart_n+restart_option: frequency-based ESMF restart trigger (via ufs.configure) *Not currently in FV3
restart_interval: just in FV3 (via model_configure) and can mean different things based on "encoding"
restart_fh: UFS wraps ESMF trigger (via model_configure). *Not currently in FV3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aerorahul - let me know if you'd like me to run a test case and save the output showing that we're writing all restarts at the same time when using IAU for all components. I can also run an fv3 standalone ci test to ensure that we are not negatively impacting atm-only runs too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aerorahul - is there any change you'd like to see with this? Or any specific tests you'd like to see output with?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. I am ok with the movement of CMEPS_RESTART_FH into the CMEPS_predet function.

@@ -134,6 +134,29 @@ common_predet(){
# Several model components share DATA/INPUT for input data
if [[ ! -d "${DATA}/INPUT" ]]; then mkdir -p "${DATA}/INPUT"; fi

# For CMEPS, CICE, MOM6 and WW3 determine restart writes
# Note FV3 has its own restart intervals
cmeps_restart_interval=${restart_interval:-${FHMAX}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a common variable if it starts w/ cmeps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you prefer it be called? This is for CMEPS, WW3, ICE, and MOM6

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not the name, but the place these are introduced.
The function CMEPS_predet IMO, would be appropriate to introduce these variables related to cmeps_ (and more general the coupled model since they share these concepts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the additional explanation. I will move it to CMEPS!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

local restart_interval_start=${cmeps_restart_interval}
local restart_interval_end=${FHMAX}
fi
CMEPS_RESTART_FH="$(seq -s ' ' "${restart_interval_start}" "${cmeps_restart_interval}" "${restart_interval_end}")"

Check warning

Code scanning / shellcheck

CMEPS_RESTART_FH appears unused. Verify use (or export if used externally). Warning

CMEPS_RESTART_FH appears unused. Verify use (or export if used externally).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any recommendations on how to fix this @DavidHuber-NOAA or @aerorahul ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add

# shellcheck disable=SC2034

before the function definition before line 731. See MOM6_predet for example.

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.

I have no concerns on this PR. Just a few comments and answers to questions.
Thanks for enabling the fortran namelist as well as using the template from ufs-weather-model.

fi
fi

# Link restart files
for (( vdate = first_ww3_restart_out; vdate <= forecast_end_cycle;
vdate = $(date --utc -d "${vdate:0:8} ${vdate:8:2} + ${restart_interval} hours" +%Y%m%d%H) )); do
ww3_restart_file="${vdate:0:8}.${vdate:8:2}0000.restart.ww3"
${NLN} "${DATArestart}/WW3_RESTART/${ww3_restart_file}" "${ww3_restart_file}"
seconds=$(to_seconds "${vdate:8:2}0000") # convert HHMMSS to seconds
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
seconds=$(to_seconds "${vdate:8:2}0000") # convert HHMMSS to seconds
seconds=$(to_seconds "${vdate:8:2}0000") # convert HHMMSS to seconds

# TODO: Need to add logic to copy restarts from DATArestart/WW3_RESTART to COMOUT_WAVE_RESTART

# Copy wave namelist from DATA to COMOUT_CONF after the forecast is run (and successfull)
${NCP} "${DATA}/ww3_shel.nml" "${COMOUT_CONF}/ufs.ww3_shel.nml"
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 fine, but beware that in seg1 will overwrite seg0's namelist.

local restart_interval_start=${cmeps_restart_interval}
local restart_interval_end=${FHMAX}
fi
CMEPS_RESTART_FH="$(seq -s ' ' "${restart_interval_start}" "${cmeps_restart_interval}" "${restart_interval_end}")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add

# shellcheck disable=SC2034

before the function definition before line 731. See MOM6_predet for example.

@@ -25,6 +25,7 @@ local SHOUR=${model_start_date:8:2}
local FHROT=${IAU_FHROT:-0}
local DT_ATMOS=${DELTIM}
local RESTART_INTERVAL="${FV3_RESTART_FH[*]}"
local RESTART_FH="${CMEPS_RESTART_FH:-" "}"
Copy link
Contributor

Choose a reason for hiding this comment

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

No. I am ok with the movement of CMEPS_RESTART_FH into the CMEPS_predet function.

Comment on lines +82 to +84
local WW3_OUTPARS=$OUTPARS_WAV
local WW3_DTFLD=$DTFLD_WAV
local WW3_DTPNT=$DTPNT_WAV
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
local WW3_OUTPARS=$OUTPARS_WAV
local WW3_DTFLD=$DTFLD_WAV
local WW3_DTPNT=$DTPNT_WAV
local WW3_OUTPARS="${OUTPARS_WAV}"
local WW3_DTFLD="${DTFLD_WAV}"
local WW3_DTPNT="${DTPNT_WAV}"

Not sure why shellnorms did not pick up on this.

# Buoy location file

if [ -f ${PARMgfs}/wave/wave_${NET}.buoys ]
then
cp ${PARMgfs}/wave/wave_${NET}.buoys buoy.loc
cp ${PARMgfs}/wave/wave_${NET}.buoys ${DATA}/ww3_points.list
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
cp ${PARMgfs}/wave/wave_${NET}.buoys ${DATA}/ww3_points.list
${NCP} "${PARMgfs}/wave/wave_${NET}.buoys" "${DATA}/ww3_points.list"

@@ -950,10 +950,6 @@ def _fcst_cycled(self):
dependencies.append(rocoto.add_dependency(dep_dict))
dependencies = rocoto.create_dependency(dep_condition='or', dep=dependencies)

Copy link
Contributor

Choose a reason for hiding this comment

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

There are 2 blank lines here. This is likely causing the pynorms test to fail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants