Feature/st4table #1124
Feature/st4table #1124
Conversation
address issue NOAA-EMC#1074 add regtest
correct duplicated variable read/write
|
@mickaelaccensi, this PR is up next, I'll put the regression tests in now! |
|
Hi @mickaelaccensi, the header mentions the matrix.comp tests were running when this was submitted. Have those completed? |
|
completed but I'm not really happy with the differences... they are many regtests with different results so I have to discuss with Fabrice first. I'll keep informed asap |
|
Hi @mickaelaccensi, okay, thank you for the update. Yes, please keep us informed when you find out more. |
|
@mickaelaccensi, with #1127 merged I'm going to start in on this PR. Do you have the matrix logs? This is a big PR with a number of expected differences, though I'm going to try to keep it moving along as Chris needs it merged for his work. |
|
@MatthewMasarik-NOAA I first have to solve the issues found in regtests mww3_test_05, ww3_ts2 and ww3_ts3 which contains too large Hs values. I'll work on it this week and keep you informed |
|
Hi @mickaelaccensi, okay I understand, thanks for the update. I'll wait to hear from you to proceed. |
Merge branch 'feature/ST4table' of https://gitlab.ifremer.fr/wave/WW3 into feature/ST4table
|
finally ! the branch is ready to be merged ! |
mickaelaccensi
requested review from
JessicaMeixner-NOAA and
MatthewMasarik-NOAA
|
@mickaelaccensi - I'll be doing the testing for this PR. Our main machine we test on is down for maintenance today, but I will submit tests first thing tomorrow. |
|
Thanks for the updates @mickaelaccensi! I will be going on leave so @JessicaMeixner-NOAA is kindly taking care of this review. |
|
@mickaelaccensi my list of not b4b tests is mostly the same as yours with a quick glance. I have a few more diffs in the ufs tests that we run and you do not and so I want to look at that. Also, since there's quite a few differences, I'm going to do a sanity test that we can reproduce answers with this PR. Our queues are pretty slow right now and despite submitting the runs last night, they have not yet started running, so this is unfortunately going to take longer than I'd hope. I'll keep you posted if I run into anything unexpected or have questions. |
| @@ -294,17 +293,18 @@ SUBROUTINE W3SPR4 (A, CG, WN, EMEAN, FMEAN, FMEAN1, WNMEAN, & | |||
| DO IK=1, NK | |||
| EB(IK) = 0. | |||
| EB2(IK) = 0. | |||
| SIGFAC=SIG(IK)**SSDSC(12) * DDEN(IK) / CG(IK) | |||
There was a problem hiding this comment.
There's never a scenario where CG(IK) =0 is there? (I'm assuming no, but if so we should have some sort of catch so we do not divide by zero).
|
@mickaelaccensi the queue times are really long right now, so unfortunately I'm still in the queue waiting. In the meantime, I went back through the regtests and things look as expected: matrixCompFull.txt I also did another check of the code and noted 2 places where we may or may not want to have a catch for divide by zero. In the meantime, I do want to make sure we can reproduce results before merging so apologies for the delay on that. Monday is a holiday here in the US, so it'll likely be 1/16 for merge right now. One other comment: Did we need to add any namelist description updates to model/inp/ww3_grid.inp? |
|
@mickaelaccensi thank you for your patience with the slow queues on this end. I have finally been able to confirm everything is b4b with itself: |
There was a problem hiding this comment.
@mickaelaccensi I'm approving this PR, however, it would be great if you could follow up on two things still:
- Confirm the two spots in w3src4md will not ever be divide by zero (a follow-up PR for work around is sufficient if this might ever happen).
- If anything needs to be added to a namelist description, please submit another PR.
There are many changes described in PR text, but I have confirmed this branch reproduces itself.
It should not happen but I'll confirm that with Fabrice
I'm working on it, I just need some additional information from Fabrice, then I'll make a PR
|
|
Thanks @mickaelaccensi ! |
|
I confirm that division by CG=0 should never happen |
Thank you!!! |
* 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]>
Pull Request Summary
remove lookup table for ST4 and clean up the ST4 code
Description
remove lookup table in ST4 to speed up computation
clean up ST4 code
correct uninitialized variables
Issue(s) addressed
Commit Message
remove lookup table for ST4 to speed up computation and clean up the ST4 code
Check list
Testing
matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt is too big to be uploaded
expected changes :
ww3_ts1 : output parameters slight changes (work_T7**)
ww3_ts2 : only mod_def since last bugfix
ww3_ts3 : only mod_def since last bugfix
ww3_ts4 : only mod_def
ww3_tp2.6 : only mod_def
ww3_tp2.6/work_pdlib : not b4b due to scotch library on Ifremer HPC (also on develop branch)
ww3_tp2.18 : only mod_def
ww3_tp2.14 : only mod_def since last bugfix
ww3_tic2.3 : only mod_def
ww3_tic1.4 : only mod_def
ww3_ta1 : only mod_def since last bugfix
mww3_test_08 : only mod_def since last bugfix
mww3_test_05 : only mod_def since last bugfix
mww3_test_03 : known not b4b
ww3_tp1.8 : output parameters changes for taw, two and foc (partially correct issue in T570)
could be expected differences :
ww3_tp2.21 : output parameters slight changes (work_b and work_mb)
ww3_tp2.17 : output parameters slight changes (work_c, work_mc, work_mc1)
ww3_tp2.15 : output parameters slight changes