First set of changes intended to fix the bug #19
Conversation
|
These changes have already been tested in the context of issue NOAA-EMC#285 (using a manual merge between branches), and regtests are being run. Comments and possible improvements are welcome. |
ukmo-juan-castillo
requested review from
JessicaMeixner-NOAA and
ukmo-ccbunney
|
Regtests behave as expected, changing only ww3_tp2.15, which reads input fields. |
|
Can you please post the matrixCompFull.out or matrixCompSummary.out as well? |
|
No problem: |
|
Instead of running matrix.comp for each test can you run "all", that way we just have 1 file that reports on every single regression test? That's what I'm used to looking at instead of all of the individual files. The code itself looks fine but I'm not an expert on these routines. My only comment/suggestion would be adding more comments in ww3_prnc with the added texts for why you are adding the other parts. If there are no changes to any of the regression tests, I see no issues with this PR. I'll wait to run the regtests on the NCEP machines when this is pushed back to the NOAA-EMC/ww3 repo. |
There was a problem hiding this comment.
Again - not an expert with this code, but it all looks okay as long as there are no changes to any existing regression tests, and fixes the issue you had on the new one being added to me. Thanks for the extra code comments! I'll run regtests when this gets pushed to noaa-emc.
|
I will send the regtests files in a while with the format you wanted. I wanted just to emphasize that this is a bug that was already present in the code. Why it arose now is because the density is used as a dividing factor in the code, so we generate division by zero errors. Zeroes and interpolation errors were already present for winds, currents, and water levels read from file, only that they did not generate crashes nor unreasonable results. I expect that with this fix the quality of the model output along the coastlines will improve, although not in coupled mode as the interpolation in this case is made via OASIS, or when the input files contain information for the whole grid domain and not only for wet points. |
There was a problem hiding this comment.
@ukmo-juan-castillo - just a comment regarding the test for "zero" fields.
the input is considered, and points that should not be taken into account in the interpolation are identified by the netcdf fill value; a subroutine is created to avoid code duplication
|
As before, the differences are in ww3_tp2.15, but they are expected: |
points instead of only one
interpolation routine
* First set of changes intended to fix the bug (#19) Fixes: NOAA-EMC#314 * Interpolation weights now correctly calculated on points next to land and BC locations. * Changes to improve the code: the possibility of reading zero values from the input is considered, and points that should not be taken into account in the interpolation are identified by the netcdf fill value; a subroutine is created to avoid code duplication * Bug fix and small simplification/optimization change (#18) * Fixes NOAA-EMC#290 (ww3_multi hanging when generating restart with IOSTYP >= 2) * Also fixes out-of-bounds array access error. * Includes some MPI optimizations * Correction to the bug fix in branch bf_multi_hang to take into account the coupled configurations, that are also affected * Small correction to the multi_hang branch: revert changes to JSEA index in w3iorsmd Co-authored-by: Juan Manuel Castillo Sanchez <[email protected]> Co-authored-by: ukmo-juan.castillo <[email protected]>
Bug fix to issue NOAA-EMC#314