Feature/skewness

[mickaelaccensi]

Pull Request Summary

add output parameters for skewness

Description

add output parameters for skewness

Commit Message

add output parameters for skewness

Check list

• Branch is up to date with the authoritative repository (NOAA-EMC) develop branch.
• Checked the checklist for a developer submitting to develop.
• If a version number update is required, checked the updating version number checklist.
• If a new feature was added, a regression test for testing the new feature is added. ww3_ts1

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
• Have the matrix regression tests been run (if yes, please note HPC and compiler)? datarmor with intel
• Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
  -the only differences are still due to pdlib on my HPC (ww3_tp2.17, ww3_tp2.21, ww3_tp2.6)
  -diff on ww3_ts1 are expected
• Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):
  matrixDiff.txt
  matrixCompSummary.txt
  matrixCompFull.txt

[MatthewMasarik-NOAA]

Hi @mickaelaccensi, thanks for the test documentation, I'll start working on this.

[ukmo-ccbunney]

Hi @mickaelaccensi.
When running the ww3_ts1 regression test in debug mode I get a floating point exception error in w3iogo:

Program received signal SIGFPE: Floating-point exception - erroneous arithmetic operation.

Backtrace for this error:
#0  0x73ACBD in _gfortrani_backtrace at backtrace.c:258
#1  0x722E20 in _gfortrani_backtrace_handler at compile_options.c:129
#2  0x721F9F in __pause_nocancel
#3  0x567844 in __w3iogomd_MOD_skewness at w3iogomd.F90:5054 (discriminator 24)
#4  0x5D5263 in __w3iogomd_MOD_w3outg at w3iogomd.F90:2355
#5  0x66EC0B in __w3wavemd_MOD_w3wave at w3wavemd.F90:2403 (discriminator 5)
#6  0x42047E in w3shel at ww3_shel.F90:2587

I'm guessing it is going to be a divide by zero issue where XN is zero?
I will try and get some more debug info for you.

Update:
I think the issue is in the SKEWNESS subroutine.
For a cold start where the spectral array A is all zero, the calculated values of XMU are also all zero (around lines 5023). This results in a XN being zero (line 5053) and being used as a denominator in the XLAMBDA calculation (5055).
It perhaps needs as a MAX applied to XN at line 5055? I'm not sure what a sensible value would be...

[MatthewMasarik-NOAA]

Thanks for catching this @ukmo-ccbunney

[mickaelaccensi]

good catch @ukmo-ccbunney !
the issue comes when XN equal 0, I've added a test on XN, I just wait for Fabrice confirmation to push it

[mickaelaccensi]

good catch @ukmo-ccbunney ! the issue comes when XN equal 0, I've added a test on XN, I just wait for Fabrice confirmation to push it

done

[ukmo-ccbunney]

Hi @mickaelaccensi
The fix to the skewness calculation working fine for me.
Just one other thing - I wonder if we should be adding Doxygen documentation to any new subroutines we add now?

[mickaelaccensi]

@ukmo-ccbunney I've added the doxygen documentation for the new subroutines
@MatthewMasarik-NOAA could add this (doxygen for new subroutines) in the PR checklist ?

[MatthewMasarik-NOAA]

@ukmo-ccbunney I've added the doxygen documentation for the new subroutines @MatthewMasarik-NOAA could add this (doxygen for new subroutines) in the PR checklist ?

Thanks @mickaelaccensi for adding doxygen for the new subroutine.

We had considered adding doxygen as a requirement at one point, though ultimately decided to wait to do so until we started building/hosting the output. We thought it would be better to wait until that functionality was available so developers would be able to use it while adding the doxygen, and not have to figure those steps out on their own. That said, for a new subroutine documentation should always be included. So for the time being, I'm fine with either the traditional WW3 subroutine documentation, doxygen documentation, or both. We can get it into doxygen form later if needed, as long as we have the info there in some form. In the not too distant future I'll be adding the doxygen build, followed by hosting it online. I can't give a firm timeline now other than to say in the not-too-distant future. However, I'll put this down as an item to give an update on at the next repo mgr meeting.

[MatthewMasarik-NOAA]

Hi @mickaelaccensi, I'm reviewing the code and saw that the two functions contained in SECONDHH: VMIN_D and VPLUS_D, could use a little more documentation. Could you add the function author and date for both of those? Also, if you have any other descriptive text to add for VPLUS_D, please add that as well. Once you add those I'll keep proceeding with the review.

[mickaelaccensi]

I've added some comments for the functions. Otherwise all the references are cited in the manual. It is manually taken from ECWAM

[MatthewMasarik-NOAA]

Thanks for adding the extra description @mickaelaccensi. When I mentioned author/date I was thinking of you as the author, with the current date, since you added the code. If you still want to add that please feel free to.

I saw there was one code update in the last commit, so just wanted to check this is complete now. Please let me know when you've made any new updates, or if the PR's ready for review now.

[mickaelaccensi]

it's almost a copy-paste from Peter Janssen code so we'll keep him as author. You can review the PR now

[MatthewMasarik-NOAA]

Ah, I see. Thanks for that clarification. I'll start processing it!

[MatthewMasarik-NOAA]

@mickaelaccensi, two PR's got merged yesterday after we chatted, could you sync this branch when you have a chance? Also, the runs for the review should complete shortly so I should be able to finish processing it later today, or tomorrow.

[mickaelaccensi]

@MatthewMasarik-NOAA the branch is synced

[MatthewMasarik-NOAA]

Code review
Pass

Testing
Pass

The only differences seen are the known non-b4b's, and the expected differences in ww3_ts1.

**********************************************************************
********************* non-identical cases ****************************
**********************************************************************
## known non-b4bs
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_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                     (17 files differ)
mww3_test_03/./work_PR1_MPI_d2                     (13 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2_c                     (16 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2_c                     (16 files differ)
mww3_test_03/./work_PR3_UNO_MPI_d2                     (17 files differ)
mww3_test_03/./work_PR2_UQ_MPI_d2                     (15 files differ)
mww3_test_03/./work_PR3_UNO_MPI_e_c                     (1 files differ)
mww3_test_03/./work_PR3_UQ_MPI_d2                     (15 files differ)
mww3_test_09/./work_MPI_ASCII                     (0 files differ)
ww3_tp2.10/./work_MPI_OMPH                     (7 files differ)
ww3_tp2.16/./work_MPI_OMPH                     (4 files differ)
ww3_tp2.6/./work_ST4_ASCII                     (0 files differ)
ww3_ufs1.3/./work_a                     (3 files differ)

## expected changes in ww3_ts1
ww3_ts1/./work_ST1                     (4 files differ)
ww3_ts1/./work_ST4_WRT                     (4 files differ)
ww3_ts1/./work_ST4_GMD                     (4 files differ)
ww3_ts1/./work_ST4                     (4 files differ)
ww3_ts1/./work_ST4_TSA                     (4 files differ)
ww3_ts1/./work_ST3                     (4 files differ)
ww3_ts1/./work_ST4_T700                     (6 files differ)
ww3_ts1/./work_ST2                     (4 files differ)
ww3_ts1/./work_ST4_T500                     (6 files differ)
ww3_ts1/./work_ST6                     (4 files differ)
ww3_ts1/./work_ST1_RWND                     (4 files differ)
**********************************************************************
************************ identical cases *****************************
**********************************************************************

• hera.intel.matrixCompSummary.txt
• hera.intel.matrixCompFull.txt
• hera.intel.matrixDiff.txt

Approved.

[MatthewMasarik-NOAA]

@mickaelaccensi thank you for adding these skewness output variables.