QP #1050

mickaelaccensi · 2023-07-28T15:35:07Z

add output parameter for k bandwidth

Pull Request Summary

correct bug fix for peakedness computation and add k bandwidth output parameter

Description

correct the computation of QP parameter
add QKK output parameter
change UST scale factor (impacts on UST and CHA precisions)
update output parameters listing to add QP and QKK and missing ones

Issue(s) addressed

#1061

Commit Message

correct the computation of QP parameter, add QKK output parameter, change UST scale factor

Testing

How were these changes tested? matrix
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) outputs added in ww3_tp2.6 and ww3_tp2.15. A dedicated regtest will be added in the next PR
Have the matrix regression tests been run (if yes, please note HPC and compiler)? yes
Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
ww3_tp2.6 : differences on cha and ust output parameters due to increase of ust scale factor
ww3_tp2.21 : crash due to nml option - see issue UG mesh crashes with ww3_multi.nml when loading input wind field #1062
ww3_tp2.10 and ww3_tp2.17 : out_grd differs but not the outputs
Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt): only tmp file because for some unknown reasons, matrix.comp get stuck on ww3_tp2.7/work_ST0 for hours!

fulldiff.tmp.txt
notIdenticalList.tmp.txt
summary.tmp.txt

add output parameter for k bandwidth

JessicaMeixner-NOAA · 2023-07-28T15:40:44Z

@mickaelaccensi two quick comments:
1: Can you put back the extra info about testing in the PR template?
2: Can you explain why the regression test is not included in this PR?

CarstenHansen · 2023-07-31T08:22:26Z

This sounds good! A long time ago (Issue #210) I considered a possible solution to this bug because I was looking at those parts of the code when a user reported it on the ww3 mailing list. I didn't really have interest in the QP (used for the Benjamin-Feir instability index). But I later found that it is still applied in some academic studies, so it is important. I remember it was a bit complex to reach a complete fix of the bug - the bug is present at more than one location in the code.

MatthewMasarik-NOAA

Hi @mickaelaccensi, just a note that there are still some questions from @JessicaMeixner-NOAA to be addressed.

mickaelaccensi · 2023-08-31T15:11:02Z

I've updated the PR message and created an issue for QP bugfix.
I've merged it with the last develop branch, I'm running now the matrix since my pdlib issue seems to be solved (both parmetis and scotch works now on my HPC).
I'll provide you the matrix.comp files for tomorrow.

MatthewMasarik-NOAA · 2023-09-06T12:39:53Z

Hi @mickaelaccensi, I don't see a regtest. Is there a reason you haven't added one?

mickaelaccensi · 2023-09-06T12:54:29Z

it is because I had to split the Fabrice branch into 3 and the regtest is designed to work with the next feature branch

JessicaMeixner-NOAA · 2023-09-06T12:59:45Z

it is because I had to split the Fabrice branch into 3 and the regtest is designed to work with the next feature branch

This is adding an output of QKK? I see this in the code, but not in the text of the PR. I get that this is part of larger work, but I'm concerned we aren't testing features when they're added. Could we add the output request of QKK to an existing regression test or 2 (maybe 1 structured 1 unstructured), even temporarily?

mickaelaccensi · 2023-09-06T14:49:21Z

added and tested in ww3_tp2.6 and ww3_tp2.15

MatthewMasarik-NOAA

Hi @mickaelaccensi, here's what I see is still outstanding:

tp2.17 - diffs in out_grd.* and restarts. Are these diffs expected?
tp2.21 - diffs in out_grd/pnt and restarts. Are these expected?

(This can be addressed at a later time)

UG mesh crashes with ww3_multi.nml when loading input wind field #1062

mickaelaccensi · 2023-09-06T15:45:42Z

I would tend to say that it is due to the bug #1062, I'm working on it right to provide a separated PR
could you try to run matrix with inp on your side to confirm that the only differences are the expected ones in ww3_tp2.6 and ww3_tp2.15 ?

MatthewMasarik-NOAA · 2023-09-06T19:20:06Z

could you try to run matrix with inp on your side to confirm that the only differences are the expected ones in ww3_tp2.6 and ww3_tp2.15 ?

Sure, I will run the matrix to get a reference on what is failing and what is not.

I would tend to say that it is due to the bug #1062, I'm working on it right to provide a separated PR

If just using ww3_multi.nml is causing the issue you saw for #1062, then that is fine and it can be resolved later. If it's a symptom of a greater issue -- if it is potentially affecting the diffs for tp2.21, tp2.17 -- then we should get to the bottom of what is going on prior to the PR being merged.

MatthewMasarik-NOAA · 2023-09-07T12:39:12Z

Update: some of the tests submitted yesterday reached time limits. Our primary RD machine was unavailable then, but is back today. I've resubmitted the runs and should have the results later today.

MatthewMasarik-NOAA · 2023-09-08T14:36:08Z

Hi @mickaelaccensi,
Here's the logs from matrix.comp

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
## known non-b4b
mww3_test_03/./work_PR1_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e_c                     (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (12 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (12 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (16 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_ufs1.3/./work_a                     (3 files differ)
##

ww3_tp2.15/./work_PR3_UQ_RHO                     (5 files differ)
ww3_tp2.15/./work_PR3_UQ_RHO_MPI                     (5 files differ)
ww3_tp2.15/./work_ST6FLX5                     (5 files differ)
ww3_tp2.15/./work_ST4FLX5                     (5 files differ)
ww3_tp2.6/./work_ST0                     (4 files differ)
ww3_tp2.6/./work_ST4                     (16 files differ)
ww3_tp2.6/./work_pdlib                     (4 files differ)

**********************************************************************
************************ identical cases *****************************
**********************************************************************

matrixCompSummary.txt
matrixCompFull.txt
matrixDiff.txt

Please add the required regtest so we can proceed with the review. Thanks

mickaelaccensi · 2023-09-08T15:00:52Z

Hi Mat,

I've already added the QP and QKK outputs in ww3_tp2.6 and ww3_tp2.15, we should find them in work_ST4

MatthewMasarik-NOAA · 2023-09-08T20:37:03Z

I've already added the QP and QKK outputs in ww3_tp2.6 and ww3_tp2.15, we should find them in work_ST4

Hi Mickael, okay I see. Thanks for that.

Then the only thing outstanding is updating the relevant scripts in /model/inp/ and model/nml/ to reflect this. Could you add documentation for QKK to these scripts?

mickaelaccensi · 2023-09-11T12:47:53Z

good catch ! it's now done

MatthewMasarik-NOAA · 2023-09-11T12:52:13Z

Great, thanks for making those changes @mickaelaccensi. I'll work on finishing the review.

MatthewMasarik-NOAA · 2023-09-12T17:51:58Z

@mickaelaccensi

Accounting of non-b4b differences

The first four files of each test below are the same (log files and 1 binary gridded output), and their differences are
accounted for by the QP and QKK variable outputs. The ww3_tp2.15 file ww3.201403.nc is also due to these variable outputs.

This leaves just the 12 additional netCDF files seen below in ww3_tp2.6/work_ST4/. The difference in these files turned out to be in the list of WW3 switches in the global_attributes. In particular, NCEP2 is replaced by NOGRB. I believe this is sneaking in at the call to run_cmake_test where the -o both option has been added, and will affect a condition within run_cmake_test that sed's NCEP2 for NOGRB.

Was the -o both option intended just for debugging, or did you want it as part of the PR?

ww3_tp2.15/./work_PR3_UQ_RHO                       (5 files differ)
ww3_tp2.15/./work_PR3_UQ_RHO_MPI                   (5 files differ)
ww3_tp2.15/./work_ST6FLX5                          (5 files differ)
ww3_tp2.15/./work_ST4FLX5                          (5 files differ)
-------------------------------------              ----------------------
  1)  log.ww3 
  2)  out_grd.ww3
  3)  ww3_ounf.out
  4)  ww3_outf.out

  5)  ww3.201403.nc



ww3_tp2.6/./work_ST0                                 (4 files differ)
ww3_tp2.6/./work_pdlib                               (4 files differ)
-------------------------------------              ----------------------
  1)  log.ww3 
  2)  out_grd.ww3
  3)  ww3_ounf.out
  4)  ww3_outf.out



ww3_tp2.6/./work_ST4                                  (16 files differ)
-------------------------------------              ----------------------
  1)  log.ww3
  2)  out_grd.ww3
  3)  ww3_ounf.out
  4)  ww3_outf.out

  5)  ww3.201008_cfd.nc
  6)  ww3.201008_cfx.nc
  7)  ww3.201008_cge.nc
  8)  ww3.201008_cha.nc 
  9)  ww3.201008_dtd.nc
 10)  ww3.201008_fc.nc
 11)  ww3.201008_hs.nc
 12)  ww3.201008_lm.nc
 13)  ww3.201008_t01.nc
 14)  ww3.201008_t02.nc
 15)  ww3.201008_t0m1.nc
 16)  ww3.201008_ust.nc

remove -o both option

mickaelaccensi · 2023-09-13T06:59:04Z

Hi @MatthewMasarik-NOAA ,

I've added QKK as output field in ww3_outf.F90 to be consistent and to allows ww3_tp2.6 and ww3_tp2.15 to output QP and QKK as ASCII output. So I've removed the -o both which was intented to output netCDF. However I don't catch your remark about the global attributes. NCEP2 is not present in the switch files for ww3_tp2.6 and ww3_tp2.15, so I don't understand why it should appear in the global attributes. Let me know.

MatthewMasarik-NOAA · 2023-09-13T13:04:13Z

I've added QKK as output field in ww3_outf.F90 to be consistent and to allows ww3_tp2.6 and ww3_tp2.15 to output QP and QKK as ASCII output.

This is sounds good, thanks for making that update.

So I've removed the -o both which was intented to output netCDF.

Great, this should remove the differences from ww3_tp2.6/work_ST4.

However I don't catch your remark about the global attributes. NCEP2 is not present in the switch files for ww3_tp2.6 and ww3_tp2.15, so I don't understand why it should appear in the global attributes.

Yes, I found this confusing too. I believe that run_cmake_test uses sed on the switchfile to change between NOGRB and NCEP2.

develop: ww3.201008_cfd.nc

bugfix/QP: ww3.201008_cfd.nc

I'll re-run the regtests and let you know the result.

MatthewMasarik-NOAA

Updated regtests

The update fixed the extra non-b4b differences that were seen in
ww3_tp2.6/work_ST4, so now all non-b4b's are accounted for.

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
## known non-b4b
mww3_test_03/./work_PR1_MPI_e                     (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UQ_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_e                     (1 files differ)
mww3_test_03/./work_PR2_UNO_MPI_d2                     (13 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (6 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (9 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (14 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (16 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (16 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_ufs1.3/./work_a                     (3 files differ)
##

ww3_tp2.15/./work_PR3_UQ_RHO             (5 files differ)
ww3_tp2.15/./work_PR3_UQ_RHO_MPI         (5 files differ)
ww3_tp2.15/./work_ST6FLX5                (5 files differ)
ww3_tp2.15/./work_ST4FLX5                (5 files differ)
ww3_tp2.6/./work_ST0                     (4 files differ)
ww3_tp2.6/./work_ST4                     (4 files differ)
ww3_tp2.6/./work_pdlib                   (4 files differ)

**********************************************************************
************************ identical cases *****************************
**********************************************************************

Code review
PASS

Testing
PASS

MatthewMasarik-NOAA · 2023-09-14T12:59:21Z

@mickaelaccensi Thank you for this nice PR. Extra thanks for the manual documentation!

mickaelaccensi · 2023-09-14T13:00:59Z

great ! let me know when you can check and merge the branch bugfix/nml, I have then 2 others PR to send about ST4 and GQM ;)

MatthewMasarik-NOAA · 2023-09-14T13:05:31Z

I will be able to start working on the bugfix/nml PR, likely tomorrow. I'm finishing some work today on #1034 and will be able to start it right after.

* Bugfix - initialised VD and VS to zero in w3srcemd. (NOAA-EMC#1037) * More efficient test for binary files in matrix.comp (NOAA-EMC#1035) * Tidy up of pre-processor directives and unused variables in w3srcemd.F90 (NOAA-EMC#1010) * Correct typo in w3srcemd.F90 pre-processor directive. (NOAA-EMC#1039) * minor bugfix for matrix grepping on keywords (NOAA-EMC#1049) * Stop masking group 1 output where icec > icen (NOAA-EMC#1019) * Doxygen documentation added, 8th subset.(NOAA-EMC#1046) * NC4 ,F90 ,XX0 switches removed from ww3_tp2.19 regtest (NOAA-EMC#1054) * CI: Fix for Intel scripts. GNU scripts updated. (NOAA-EMC#1064) * correct the computation of QP parameter, add QKK output parameter, change UST scale factor (NOAA-EMC#1050) * correct issue with ww3_multi when requesting restart2 and using nml file instead of inp file (NOAA-EMC#1070) * correct calendar for track netcdf output (NOAA-EMC#1079) * Fix missing mod_def.ww3 file in multigrid regression tests for track output (NOAA-EMC#1091) * STAB3: fix cmake build for ST4 or ST3 (NOAA-EMC#1086) * new feature to output out_grd.ww3, out_pnt.ww3 and mod_def.ww3 both in binary and ascii format using switch ASCII. (NOAA-EMC#1089) * Update local unit number arrays (NDS, MDS) to be same size of array defined in w3odatmd (size=15). Also, defined unit numbers for NDS(14) and NDS(15). (NOAA-EMC#1098) * Removed code referencing PHIOC in output section for PHICE in ww3_ounf (NOAA-EMC#1093) * implementation of the GQM (Gaussian Quadrature Method) to replace the DIA in NL1 or NL2. (NOAA-EMC#1083) * update logic to ensure you are not accessing uninitialized dates (NOAA-EMC#1114) * Initialised S and D arrays in W3SDB1 before potential early return if zero energy. (NOAA-EMC#1115) * ww3_ounp.F90: x/y units attribute corrected from 'm' to 'km' (NOAA-EMC#1088) * Bugfix: Assign unit numbers to ASCII gridded/point output in multi-grid mode. (NOAA-EMC#1118) * correct bugs to run correctly GQM implementation (NOAA-EMC#1127) * Adding documentation to w3iopo() in preparation for code for NOAA-EMC#682. (NOAA-EMC#1131) * NCEP regtest module updates: uses spack-stack/1.5.0, includes scotch/7.0.4 (NOAA-EMC#1137) * Minor update to ncep regtests (NOAA-EMC#1138) * Updated intel workflow to install oneapi compilers from new location. (NOAA-EMC#1157) * Add unit test for points I/O code. (NOAA-EMC#1158) * Update Intel CI (relocate /usr/local; ensure intel-oneapi-mpi; use ubuntu-latest) (NOAA-EMC#1161) * remove lookup table for ST4 to speed up computation and clean up the ST4 code (NOAA-EMC#1124) Co-authored-by: Fabrice Ardhuin <[email protected]> * initialize USSP_WN for mod_def (NOAA-EMC#1165) * Introduce IC4M8 and IC4M9 to WW3 (NOAA-EMC#1176) * clean up and add ST4 variables (NOAA-EMC#1181) * w3fld1md.F90: fix divide by zero in CRIT2 parameter (NOAA-EMC#1184) * ww3_prnc.F90: fix out-of-scope grid index write statement (NOAA-EMC#1185) * Bugfix: address potential divide-by-zero in APPENDTAIL (NOAA-EMC#1188) Co-authored-by: Denise Worthen <[email protected]> * Provide initial drying of cells with depth < ZLIM for SMC grid. (NOAA-EMC#1192) * Output OMP threading info to screen when running ww3_shel/ww3_multi compiled with the OMPG switch. Also fixes truncation of build.log when running run_cmake_build. (NOAA-EMC#1191) * Added screen output showing number of threads when OMP enabled. * update build to get more info in logs (NOAA-EMC#46) --------- Co-authored-by: Jessica Meixner <[email protected]> * update run_cmake_test to catch build errors and exit (NOAA-EMC#1194) * fix merge conflicts * Fix gustiness bug, as suggst by Pieter * Change USTARsigma to WAM implementation --------- Co-authored-by: Chris Bunney <[email protected]> Co-authored-by: Mickael Accensi <[email protected]> Co-authored-by: Benoit Pouliot <[email protected]> Co-authored-by: Matthew Masarik <[email protected]> Co-authored-by: Ghazal-Mohammadpour <[email protected]> Co-authored-by: Jessica Meixner <[email protected]> Co-authored-by: Biao Zhao <[email protected]> Co-authored-by: Edward Hartnett <[email protected]> Co-authored-by: Alex Richert <[email protected]> Co-authored-by: Fabrice Ardhuin <[email protected]> Co-authored-by: W. Erick Rogers <[email protected]> Co-authored-by: Denise Worthen <[email protected]> Co-authored-by: Camille Teicheira <[email protected]>

correct bug fix for peakedness computation

28011ee

add output parameter for k bandwidth

mickaelaccensi requested a review from MatthewMasarik-NOAA July 28, 2023 15:35

MatthewMasarik-NOAA requested changes Aug 4, 2023

View reviewed changes

Merge branch 'develop' into bugfix/QP

2beb1c8

mickaelaccensi requested a review from MatthewMasarik-NOAA September 6, 2023 09:17

add QP and QKK to output for regtests tp2.6 and tp2.15

53de47f

MatthewMasarik-NOAA requested changes Sep 6, 2023

View reviewed changes

update output parameters listing to add QP and QKK and missing ones

d16e31c

mickaelaccensi requested a review from MatthewMasarik-NOAA September 11, 2023 12:48

mickaelaccensi mentioned this pull request Sep 12, 2023

Bugfix/nml #1070

Merged

4 tasks

add QKK in ww3_outf

9118d1e

remove -o both option

MatthewMasarik-NOAA approved these changes Sep 14, 2023

View reviewed changes

MatthewMasarik-NOAA merged commit 6b9edfa into NOAA-EMC:develop Sep 14, 2023
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QP #1050

QP #1050

mickaelaccensi commented Jul 28, 2023 •

edited

Loading

JessicaMeixner-NOAA commented Jul 28, 2023

CarstenHansen commented Jul 31, 2023

MatthewMasarik-NOAA left a comment

mickaelaccensi commented Aug 31, 2023

MatthewMasarik-NOAA commented Sep 6, 2023

mickaelaccensi commented Sep 6, 2023

JessicaMeixner-NOAA commented Sep 6, 2023

mickaelaccensi commented Sep 6, 2023

MatthewMasarik-NOAA left a comment •

edited

Loading

mickaelaccensi commented Sep 6, 2023

MatthewMasarik-NOAA commented Sep 6, 2023

MatthewMasarik-NOAA commented Sep 7, 2023

MatthewMasarik-NOAA commented Sep 8, 2023

mickaelaccensi commented Sep 8, 2023

MatthewMasarik-NOAA commented Sep 8, 2023

mickaelaccensi commented Sep 11, 2023

MatthewMasarik-NOAA commented Sep 11, 2023

MatthewMasarik-NOAA commented Sep 12, 2023

mickaelaccensi commented Sep 13, 2023

MatthewMasarik-NOAA commented Sep 13, 2023

MatthewMasarik-NOAA left a comment

MatthewMasarik-NOAA commented Sep 14, 2023

mickaelaccensi commented Sep 14, 2023

MatthewMasarik-NOAA commented Sep 14, 2023

QP #1050

QP #1050

Conversation

mickaelaccensi commented Jul 28, 2023 • edited Loading

Pull Request Summary

Description

Issue(s) addressed

Commit Message

Testing

JessicaMeixner-NOAA commented Jul 28, 2023

CarstenHansen commented Jul 31, 2023

MatthewMasarik-NOAA left a comment

Choose a reason for hiding this comment

mickaelaccensi commented Aug 31, 2023

MatthewMasarik-NOAA commented Sep 6, 2023

mickaelaccensi commented Sep 6, 2023

JessicaMeixner-NOAA commented Sep 6, 2023

mickaelaccensi commented Sep 6, 2023

MatthewMasarik-NOAA left a comment • edited Loading

Choose a reason for hiding this comment

mickaelaccensi commented Sep 6, 2023

MatthewMasarik-NOAA commented Sep 6, 2023

MatthewMasarik-NOAA commented Sep 7, 2023

MatthewMasarik-NOAA commented Sep 8, 2023

mickaelaccensi commented Sep 8, 2023

MatthewMasarik-NOAA commented Sep 8, 2023

mickaelaccensi commented Sep 11, 2023

MatthewMasarik-NOAA commented Sep 11, 2023

MatthewMasarik-NOAA commented Sep 12, 2023

Accounting of non-b4b differences

mickaelaccensi commented Sep 13, 2023

MatthewMasarik-NOAA commented Sep 13, 2023

MatthewMasarik-NOAA left a comment

Choose a reason for hiding this comment

Updated regtests

MatthewMasarik-NOAA commented Sep 14, 2023

mickaelaccensi commented Sep 14, 2023

MatthewMasarik-NOAA commented Sep 14, 2023

mickaelaccensi commented Jul 28, 2023 •

edited

Loading

MatthewMasarik-NOAA left a comment •

edited

Loading