Duplicated oasis get calls #858
Conversation
|
Hi @ukmo-juan-castillo - did you run the ww3_tp2.14 regtest with your fix prior to modifying the test? I just wanted to check that it was producing the same answers prior to your update. I am going to run the full regtest suite today. |
|
@ukmo-juan-castillo can you please merge in the noaa-emc/develop branch to your branch for this PR so that we can get the CI fixes here as well? Thanks! |
I've done this now @JessicaMeixner-NOAA . |
|
@ukmo-juan-castillo - I am running the regression tests for this change and the model is hanging on the new If I revert the change to that regtest (setting |
|
@ukmo-juan-castillo I've just pushed a small correction to the |
There was a problem hiding this comment.
@ukmo-juan-castillo
I've rerun the ww3_tp2.14 regression tests.
There are some differences in the nout.00000 files in all tests (see specific remarks below).
The OASACM6 test obviously has more difference due to the addition of the density field.
However, there are a couple of issues.
- Firstly, the
density.ncfield used as input toww3_prncis for a different region (Adriatic Sea) so the input density.ww3 is all zero. - Secondly, in the output file
ww3.201109.nc, the wind speeds are all zero (maybe a result of the density being zero?), so the resulting wave field is also zero.
I think we will need to add a new density.nc file for this regtest. It could be a small and very coarse resolution file with homogeneous values just to provide an input.
|
I have run the full regression test matrix and get the following expected differences to but there are also a few unexpected differences that remain even after rerunning:
I am a bit suspicious that this is some oddity to our system, so I wait to see what the results from NOAA's regteests are . Certainly it makes no sense that the |
…acm6 Updated version number for ww3_from_ftp tar file
@ukmo-ccbunney I've submitted the regtests for this. We will report back on the differences you reported. |
|
@ukmo-ccbunney can you update the version number to 7.14.1 instead of 7.12.7? The density.nc file should be there now in that file. Also, would you mind merging with the latest noaa-emc/develop? I'm starting regtests now (Matt is on leave). |
Done! (online via github.dev, which was very exciting!) |
|
@ukmo-ccbunney I ran the regtests yesterday and I didn't get either of the issues you mentioned. I do get mod_def diffs for unstructured grids on a regular basis (although not for tp2.7 oddly enough), so I think that's just the known issue that we really need to get fixed. I did just go and check the mww3_test_02/./work_PR3_UQ_MPI_c_c/ww3_multi.out file and it looks like we have some silent failures in that test that also exist in the develop branch. I'm getting: " integration stalled: num_subseg exceeded limit" I'm going to re-run the regtests today just to do a sanity check on the tar-file, but otherwise think this will be ready to merge in this afternoon unless we need to wait for @mickaelaccensi to review and approve as well. |
There was a problem hiding this comment.
**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
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_d2 (16 files differ)
mww3_test_03/./work_PR1_MPI_d2 (12 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c (9 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c (16 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2 (16 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2 (14 files differ)
mww3_test_03/./work_PR3_UQ_MPI_e (1 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e_c (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.14/./work_OASACM6 (10 files differ)
ww3_tp2.14/./work_OASACM5 (2 files differ)
ww3_tp2.14/./work_OASACM4 (2 files differ)
ww3_tp2.14/./work_OASACM (2 files differ)
ww3_tp2.14/./work_OASICM (2 files differ)
ww3_tp2.14/./work_OASACM2 (2 files differ)
ww3_tp2.14/./work_OASOCM (2 files differ)
ww3_tp2.16/./work_MPI_OMPH (4 files differ)
ww3_tp2.17/./work_ma (1 files differ)
ww3_tp2.17/./work_ma1 (1 files differ)
ww3_ufs1.3/./work_a (3 files differ)
|
@ukmo-ccbunney apologies for the slowness on the regtests on our end. The second time I ran this branch, I had a few " 0 diffs" but looking into a case or two, it doesn't seem to be anything concerning. @MatthewMasarik-NOAA or I will follow up with the mww3_test_02/./work_PR3_UQ_MPI_c_c test and make an issue to document this regression tests with the silent failure on our end and actual error on yours. @mickaelaccensi --- if you'd like to continue to review this PR, please do and if you find any issues or have any concerns we'll address them when you raise them. |
Great - Thanks Jessica. |
Pull Request Summary
Bug fix for a problem when happens in certain circumstances (using driving data coming from oasis or from file in certain combinations), when oasis_get is called twice for the same field and time, causing OASIS to crash
Description
It was discovered that, when reading water levels from file and driving the wave model from winds received via coupling, and coupling at T+0, the oasis_get call for winds at time start zero happens twice, which causes OASIS to crash because as expected the model that sends the winds only sends the fields once at each coupling step.
This is caused by a series of things: water levels are read before winds, and they are not time interpolated. This causes that the time step is not updated the first time the input fields are updated, so the input fields are updated twice the first time step.
The fix here proposed makes sure that oasis_get is called only once if the input fields are updated twice for the first time step.
To reproduce this error, I modified one ww3_tp2.14 test. The reviewer is encouraged to run the develop branch with the modified regtest to demonstrate that a bug is actually present, and that the changes in this branch fixes it. Given that this particular regtest is changed, it is expected to cause a change in answers in just that test.
Labels that should be added: bug
Issue(s) addressed
Commit Message
Fix duplicated calls to oasis_get happening in some mixed forced/coupled configurations
Check list
Testing