Skip to content

1.5.9 zbry clean#138

Open
jkousal32 wants to merge 89 commits into
ecmwf-ifs:develop-1.6from
jkousal32:1.5.9_zbry_clean
Open

1.5.9 zbry clean#138
jkousal32 wants to merge 89 commits into
ecmwf-ifs:develop-1.6from
jkousal32:1.5.9_zbry_clean

Conversation

@jkousal32

@jkousal32 jkousal32 commented Apr 29, 2026

Copy link
Copy Markdown
Collaborator

Description

BYDRZ alternative source term package (ST6-like in the WAVEWATCH III convention)

At this point, I believe all is up to the ecwam standard except for the noted BYDRZ TODO, which I will address soon (EDIT*** completed this TODO now)

I believe this is now a good point to get some more eyes on this code.

Very minor changes in ifs-source and ifs-scripts to support those in ecWAM.

These PRs (across ifs-source, ifs-scripts, ecWAM) should be merged together.

Update: these will go into 51R1 together, once declared, with ecwam 1.6

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

@jkousal32 jkousal32 self-assigned this Apr 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds the BYDRZ (ST6-like) alternative wave physics package to ECWAM, including new wind-input and dissipation terms, and exposes the configuration via run scripts/tests.

Changes:

  • Introduces a new physics branch (IPHYS=2) with BYDRZ wind input, dissipation, swell dissipation, stress diagnostics, and air–sea interaction handling.
  • Extends core interfaces (e.g., SINFLX/SDISSIP/SINPUT/AIRSEA) to pass additional state needed by the new physics (e.g., CGROUP, CHRNCK, gustiness outputs).
  • Adds/updates test configurations and run-script parameters to exercise the new physics options.

Reviewed changes

Copilot reviewed 33 out of 35 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
tests/etopo1_oper_an_fc_O48_cy50r1_bydrz.yml Adds a BYDRZ-specific regression test config and expected validation stats.
tests/etopo1_oper_an_fc_O48_cy50r1.yml Extends baseline test outputs to include additional stress-related fields.
src/ecwam/yowstat.F90 Adds new runtime switches/parameters (e.g., IPHYS2_AIRSEA, LLLOWWINDS, ZCDFAC).
src/ecwam/yowphys.F90 Adds BYDRZ physics parameters (including water viscosity constant and BYDRZ tunables).
src/ecwam/yowpcons.F90 Centralizes Euler’s constant (GAMMA_E) in constants module.
src/ecwam/yowfred.F90 Adds allocatable frequency-grid helper arrays for BYDRZ extensions.
src/ecwam/wsigstar.F90 Extends gustiness routine to also return SIG_U10.
src/ecwam/wdfluxes.F90 Updates calls to SINFLX/SDISSIP to match new signatures.
src/ecwam/w_maxh.F90 Switches to module-provided GAMMA_E constant.
src/ecwam/userin.F90 Wires new namelist/state variables and prints BYDRZ-related settings.
src/ecwam/tauwindsxy.F90 New helper to integrate wind-input spectra into stress components.
src/ecwam/tau_wave_atmos.F90 New routine to compute stress from negative input (waves → atmosphere), incl. HF extension.
src/ecwam/swldissip_bydrz.F90 New BYDRZ swell dissipation source term.
src/ecwam/sinput_jan.F90 Updates WSIGSTAR call-site for new SIG_U10 output and comment cleanup.
src/ecwam/sinput_ard.F90 Updates WSIGSTAR call-site for new SIG_U10 output.
src/ecwam/sinput.F90 Extends SINPUT interface (CGROUP, CHRNCK; Z0M/UFRIC intent changes).
src/ecwam/sinflx_bydrz.F90 New BYDRZ wind input + stress balance + gustiness + PHIWA handling.
src/ecwam/sinflx_ard_jan.F90 Refactors existing SINFLX logic into a dedicated ARD/JAN implementation.
src/ecwam/sinflx.F90 Dispatches to SINFLX_ARD_JAN vs SINFLX_BYDRZ based on IPHYS.
src/ecwam/setwavphys.F90 Adds default parameterization for IPHYS=2 and IPHYS2_AIRSEA options.
src/ecwam/sdissip_bydrz.F90 New BYDRZ dissipation source term.
src/ecwam/sdissip.F90 Dispatches dissipation to BYDRZ implementations for IPHYS=2.
src/ecwam/outbeta.F90 Extends local variables for new flux/stress-related diagnostics.
src/ecwam/mpuserin.F90 Adds defaults and documentation for new IPHYS2_AIRSEA/LLLOWWINDS/ZCDFAC controls.
src/ecwam/lfactor.F90 New reduction-factor solver to enforce stress constraints via spectral reduction.
src/ecwam/initmdl.F90 Adds BYDRZ-specific frequency-space setup and HF extension grid allocation.
src/ecwam/implsch.F90 Adjusts SINFLX call count and updated interfaces for new physics.
src/ecwam/frcutindex.F90 Minor whitespace-only change.
src/ecwam/calcphiwa.F90 New energy-flux-from-input computation (PHIWA) helper.
src/ecwam/airsea_jan.F90 New JAN-specific AIRSEA implementation extracted from AIRSEA.
src/ecwam/airsea_bydrz.F90 New BYDRZ-specific AIRSEA implementation.
src/ecwam/airsea.F90 Refactors AIRSEA into a dispatcher with optional HALP/RNFAC for JAN branch.
src/ecwam/CMakeLists.txt Registers new source files in the build.
share/ecwam/scripts/ecwam_run_model.sh Exposes IPHYS2_AIRSEA and LLLOWWINDS config into the generated namelist.
.gitignore Ignores x*.sh files.
Comments suppressed due to low confidence (1)

src/ecwam/yowpcons.F90:1

  • GAMMA_E is a physical/mathematical constant and should be declared as a PARAMETER to prevent accidental runtime modification and to enable compiler optimizations. Consider switching this to REAL(KIND=JWRB), PARAMETER :: GAMMA_E = ....

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ecwam/swldissip_bydrz.F90
Comment thread src/ecwam/initmdl.F90 Outdated
Comment thread src/ecwam/airsea.F90 Outdated
Comment thread src/ecwam/tau_wave_atmos.F90
Comment thread src/ecwam/sinput_jan.F90 Outdated
Comment thread src/ecwam/airsea_jan.F90 Outdated
Comment thread src/ecwam/calcphiwa.F90 Outdated
Comment thread src/ecwam/mpuserin.F90
Comment thread src/ecwam/setwavphys.F90 Outdated
Comment thread src/ecwam/setwavphys.F90 Outdated
@jkousal32

Copy link
Copy Markdown
Collaborator Author

@jrbidlot @awnawab heads up, have now completed the last TODO. Now I believe all is up to the ecwam standard and good to go!

@awnawab
awnawab changed the base branch from main to develop-1.6 May 7, 2026 07:25
@awnawab

awnawab commented May 7, 2026

Copy link
Copy Markdown
Collaborator

As discussed offline, I've retargeted this PR to develop-1.6, which we can use to collect 51R1 contributions. We can also ignore the failing tests, these are due to failing loki builds, which I'll mop up after this PR is merged.

@awnawab awnawab left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for this @jkousal32 🙏 I can't comment on the science but technically this is implemented quite well, I've just left a few minor comments which would be nice to address before we merge this. Just for my own understanding, does this new physics package offer similar forecasting skill at a reduced computational cost? I seem to remember hearing something about that, although it's quite possible I hallucinated that 😅

Once the above points are addressed, I would be happy to merge this, and just to reiterate don't worry about the failing loki builds. The cpu hpc-ci build doesn't use loki preprocessing, and this test passes.

After this PR, it would be good to have a discussion offline about whether we do want to continue to maintain three physics packages in the long term or whether one of these could potentially eventually be retired. Secondly, I'd like to get your thoughts on possibly have the branching between the various packages at a much higher level outside the OpenMP loop. Although this may lead to a bit of code duplication, we would end up with simpler and potentially faster (on CPU and especially on GPU) compute kernels. Let's discuss further on our dedicated waves channel 😊

! Wind height
ZNLEV = 10._JWRB

!$loki inline

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you please remove this new loki inline directive? This particular one is misplaced and should preceed a call, but in general, the performance impact of inlining is not easily determined a priori, so we shouldn't add these by default.

!* 3. DETERMINE ROUGHNESS LENGTH (if needed).
! ---------------------------

!$loki inline

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same comment here about inlining.

#include "taut_z0.intfb.h"
#include "z0wave.intfb.h"

INTEGER(KIND=JWIM), INTENT(IN) :: KIJS, KIJL, ICODE_WND, IUSFG

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The indentation is a little off here. Could we please align the declarations and the subroutine body itself with the module imports?

Comment thread src/ecwam/calcphiwa.F90

IMPLICIT NONE

REAL(KIND=JWRB), DIMENSION(NANG,NFRE), INTENT(IN) :: SPOS ! POS Sin(sigma) in [m2/Hz]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same comment here about indentation.

Comment thread src/ecwam/calcphiwa.F90
!/ 0) --- split integral into low/high frequency contributions ------------- /
!
!
! Th=2pi,f=inf Th=2pi,f=FR(NFRE) Th=2pi,f=inf

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice ASCII math 🤓👏

#include "calcphiwa.intfb.h"


INTEGER(KIND=JWIM), INTENT(IN) :: ICALL !! CALL NUMBER.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same comment here about indentation.


IMPLICIT NONE

INTEGER(KIND=JWIM), INTENT(IN) :: KIJS, KIJL

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Indentation please.

! Determine value of spectrum at NFRE (i.e. at highest frequency).
! - Note, direction dimension must remain

ZA_SX = SX(:,NFRE)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Explicit bounds on the LHS please.


#include "tauwindsxy.intfb.h"

REAL(KIND=JWRB), DIMENSION(NANG,NFRE), INTENT(IN) :: S ! Sin(sigma) in [m2/rad-Hz]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Indentation please.

Comment thread src/ecwam/tauwindsxy.F90

IMPLICIT NONE

REAL(KIND=JWRB), INTENT(IN) :: SDENSX_IN(*), SDENSY_IN(*)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we need assumed size declarations here, would assumed shape not suffice? Also indentation please.

@jkousal32

jkousal32 commented May 18, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @awnawab
No, unfortunately no speed up brought at this stage relative to 50r1.
I did get a big speed up relative to my first implementation though, which might be what you are thinking of:
Screenshot 2026-05-18 at 14 09 09
This was a first (big) pass at trying to reduce the cost. Perhaps more speed up can be found. Timings are for the source term operations in the O48 test.

Thanks for your other comments. Will address soon!

Plans for further offline discussion sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants