Feature/gqm #1083
Merged
Feature/gqm #1083
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mickaelaccensi I'm on board with updating the version number. It has been a while after all. Best I remember this causes additional changes to answers, so perhaps it'd be best to have it as a follow-up PR to have clean tests here. |
others will be added for next PR (ST4table)
|
Hi @mickaelaccensi, there's no one ahead of you so I'll start the tests and get the review going. Thank you kindly for providing the testing logs and expected differences. I should be able to get it wrapped up Wed -- giving some time to verify the changes -- but Thursday at the latest. |
|
Hi @mickaelaccensi, I wanted to touch base briefly and give two updates. First on the time frame I gave earlier, I apologize, at the time I was not factoring in that I would be on leave for a half day today and tomorrow. I'll be back Friday. Also, I wanted to mention that Jessica and I are pretty swamped at the moment with preparations for the wave workshop next week, as well as a potential federal government shutdown starting monday as well. So, just an FYI that processing may be a little slower and we are playing things by ear with respect to a shutdown. I tried to squeeze some reviewing in before leaving. Here are some comments so far:
matrix.comp output |
mickaelaccensi
added
new feature
mod_def change
|
@MatthewMasarik-NOAA , |
|
@mickaelaccensi thanks for the update to fix the ww3_ts1 crash. At this time - it seems like we have a remaining issue about the PDLIB cases. It's a little tricky, because while it's likely not caused by this update - especially since you even have b4b issues on your machine that we do not see. Thanks for making the issue about that. I do see PR #1037 has some initialized variables that could be worth a try to see if that helps with your issues you're seeing in develop and maybe it could even help here? We need to do some more testing and attempts at fixing before moving forward with this PR. I know this is high priority for you and we're all exciting about it's developments, but at this time this PR is not passing regression testing as expected so we need to work to see if we can resolve those issues. I unfortunately am likely unable to help for a while, but I will try to find some time to look through the code today and see what I might be able to help find. Thanks in your advance for your help and patience with this. |
|
Since the ww3_tp2.6 issue is linked to SCOTCH library, could you merge this branch ? |
|
@mickaelaccensi no, this branch cannot be merged until issues are resolved, we cannot put in new issues into the develop branch. Have you tried #1037 to see if that helps? |
remove GQM from SNL2
|
can you merge it now ? |
|
@mickaelaccensi I'll be happy to try running this branch using METIS instead of SCOTCH on Monday. It will be an interesting data point, and potentially give us a clue of how to solve this issue, but either way, we still need to resolve why this test is failing with this branch currently. |
|
I thought it was b4b on your matrix based on your matrix output : |
|
From the PR header I've been under the impression that this test was not expected to differ, but did, and pdlib is not an explanation for differences. And I ran the regtests, and I got non-b4b's, and our discussions hadn't resolved why. If it is just those files, and that is expected, then that should be OK for that regtest. There's been at least one push since I ran the regtests last, so that probably needs to be redone at this point. I can't get to any further analysis until the beginning of the week though, but happy to re-review the results at that point and hopeful things will be closer to resolution. |
|
I rephrased the PR header because this is not appearing in my feature branch but the develop branch. The only reason I didn't noticed it earlier was because I only recently installed scotch on my HPC, so before this test was just crashing (as for Ukmet HPC since no scotch library is installed on it) |
|
Okay, got it. Thanks for that context, @mickaelaccensi |
|
Hi @mickaelaccensi, with #1089 merged in, this PR is next up again. Could you please sync this branch when you're able? You'll have one merge conflict to resolve, which is due to the feature/ascii updates. In the meantime I'm working on running the regtests (with the latest updates merged in locally). My first attempt at resolving the merge conflict resulted in a bunch of crashes in the matrix scripts. Hopefully I just made the wrong choice.. I'm going to try resolving with the alternate block, then running again. I'll plan to post the results of those runs tomorrow morning. |
|
merged and conflicts solved ! |
|
Thank you @mickaelaccensi. For the runs from last night, there were many unexpected differences in the matrix.comp. They could be explained by @ukmo-ccbunney's bug find (#1097) though, which he is also working on a fix for. We should get that in next, then see if there if there are any remaining differences. |
@MatthewMasarik-NOAA would it be possible to post what tests are different? |
|
@JessicaMeixner-NOAA, sure, there's quite a few though. (I should note the merge-conflict may have been not been resolved identically, though I haven't carefully checked through it yet) Here's the summary and files: |
|
@MatthewMasarik-NOAA thanks for sharing. The 0 diff files is a bit odd. Hopefully new runs or other bug fixes will help. Let me know if there's anything I can do in terms of helping with testing. |
|
@mickaelaccensi we just merged #1098, so this PR is back at the top. I'll submit the runs now. If they come back with a clean report it will go in next. If there are any remaining non-b4b's to resolve, we'll try to get #1093 in while they are fixed. I'm going to start those runs (#1093) now as well to be a back up. |
4 tasks
|
I've merged the last develop PR into feature/GQM branch. I don't know if you need to rerun the matrix to have consistent outputs |
|
Thanks Mickael. I did a manual merge with the latest commits just so I could get runs started yesterday. The matrix.comp is running now, so we should have results to look at shortly. I can re-run them if need be. |
|
Hi @mickaelaccensi, here's the diffs from the runs. There are the known non-b4b cases up top, then blocks of tests that you've given explanations for the diffs, and then below the dashed line, a handful of tests that were not listed as expected ( matrixCompSummary.txt matrix.comp |
|
@MatthewMasarik-NOAA @mickaelaccensi Can you help me understand why are some mod_defs but not all different and some grid.out diffs and others not? |
|
yes it is expected. The reason why mod_def.ww3 changes is because of the new variables added in NL1, so only regtests with NL1 actived are affected by this change. If there are some regtests (ww3_tnc1, ww3_tr1) with a different mod_def.ww3 but the same ww3_grid.out is because the switch O0 is not present in the switch file and so the namelists values are not outputed. |
Great. Thank you for that explanation @mickaelaccensi. I'm going to proceed with the approval now. |
MatthewMasarik-NOAA
approved these changes
Oct 19, 2023
There was a problem hiding this comment.
Code review
Pass
Testing
Pass
Matrix.comp summary and the log files were attached to: #1083 (comment) . @mickaelaccensi's reply (#1083 (comment)) explains remaining differences, so all differences are accounted for.
Approved.
|
@mickaelaccensi, thank you very much for this big piece of work related to nonlinear interaction parameterization! |
miguelsolanocordoba
added a commit
to wavespotter/WW3
that referenced
this pull request
Apr 19, 2024
* 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 <fabrice.ardhuin@ifremer.fr> * 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 <denise.worthen@noaa.gov> * 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 <jessica.meixner@noaa.gov> * 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 <48915820+ukmo-ccbunney@users.noreply.github.com> Co-authored-by: Mickael Accensi <49198861+mickaelaccensi@users.noreply.github.com> Co-authored-by: Benoit Pouliot <51411504+benoitp-cmc@users.noreply.github.com> Co-authored-by: Matthew Masarik <86749872+MatthewMasarik-NOAA@users.noreply.github.com> Co-authored-by: Ghazal-Mohammadpour <124626872+Ghazal-Mohammadpour@users.noreply.github.com> Co-authored-by: Jessica Meixner <jessica.meixner@noaa.gov> Co-authored-by: Biao Zhao <zhaobiaodeyouxiang@163.com> Co-authored-by: Edward Hartnett <38856240+edwardhartnett@users.noreply.github.com> Co-authored-by: Alex Richert <82525672+AlexanderRichert-NOAA@users.noreply.github.com> Co-authored-by: Fabrice Ardhuin <fabrice.ardhuin@ifremer.fr> Co-authored-by: W. Erick Rogers <156342000+ErickRogers@users.noreply.github.com> Co-authored-by: Denise Worthen <denise.worthen@noaa.gov> Co-authored-by: Camille Teicheira <cteicheira@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Summary
Description
implementation of the GQM (Gaussian Quadrature Method) to replace the DIA.
It can be activated in NL1 or NL2. Default parameterization is still DIA. To enable GQM, IQTYPE must be negative in the namelist.
Issue(s) addressed
n/a
Commit Message
implementation of the GQM (Gaussian Quadrature Method) to replace the DIA in NL1 or NL2.
Check list
@MatthewMasarik-NOAA and @JessicaMeixner-NOAA , maybe we should set a new version number for this feature ?
Testing
How were these changes tested?
matrix
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
ww3_ts1 with namelists_ST4_T707.nml and namelists_ST4_T713.nml
Have the matrix regression tests been run (if yes, please note HPC and compiler)?
datarmor with intel compiler
Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
expected differences :
ww3_ts1 : additional tests in GQM branch (added after this matrix.comp)
ww3_ts1 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_ts2 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_ts3 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_tp2.7 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_tp2.6 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_tp2.21 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_tp2.17 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_tp2.15 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_tp2.14 : expected diff only in ww3_grid.out and mod_def.ww3
ww3_ta1 : expected diff only in ww3_grid.out and mod_def.ww3
mww3_test_08 : expected diff only in ww3_grid.out and mod_def.ww3
mww3_test_05 : expected diff only in ww3_grid.out and mod_def.ww3
not expected differences :
mww3_test_03 : known not b4b
ww3_tp2.6/work_pdlib : not b4b due to scotch library on Ifremer HPC (also on develop branch)
matrixCompFull.txt
matrixCompSummary.txt
matrixDiff.txt