NOAA-EMC / WW3

WAVEWATCH III
Other
265 stars 542 forks source link

Inconsistent rank of spectral array #1208

Open ukmo-ccbunney opened 7 months ago

ukmo-ccbunney commented 7 months ago

Describe the bug [Not a bug per se, but more of an issue around consistency that does complicate things like subroutine overloading]

Most of the source term functions receive the spectral density (often A or SPEC) as a rank 1 array with the dimensions NSPEC where NSPEC = NK * NTH However, there are several places, especially in the point output post-processors, where the array that is passed in is defined as rank 2 array with the dimensions NTH, NK, rather than a rank 1 array of size NSPEC.

Fortran (surprisingly) seems to allow this and is happy to quietly handle a 2D array as a flattened 1D array.

However, Fortran gets a lot more picky when you overload subroutines when using an INTERFACE block (something I have been looking into with the ST4 source package) which makes things more complicated.

Are there any objections to changing the use of 2D spectral arrays to 1D versions instead? This would mainly affect the point output processor programs. Apart from the issues it causes with subroutine overloading, it feels more aesthetically consistent with the internal 1D representation of spectral arrays in the rest of the model.

Example: ww3_ounp calls W3SIN4: https://github.com/NOAA-EMC/WW3/blob/f66b6d46ad8204a74ad4112ef39d5a230525f8c8/model/src/ww3_ounp.F90#L2190 where A is defined as a 2D array: https://github.com/NOAA-EMC/WW3/blob/f66b6d46ad8204a74ad4112ef39d5a230525f8c8/model/src/ww3_ounp.F90#L1630

but W3SIN4 expects a 1D array: https://github.com/NOAA-EMC/WW3/blob/f66b6d46ad8204a74ad4112ef39d5a230525f8c8/model/src/w3src4md.F90#L535

mickaelaccensi commented 7 months ago

I agree, it seems sensible to have the same 1D array in all the code to avoid fortran to decide by itself what to do

MatthewMasarik-NOAA commented 7 months ago

In general I lean towards consistency, so I will likely think this is sensible too. I'd like to discuss with Jessica first though, when she's back early next week, just to make sure there isn't anything I've overlooked. I'll post back again then.

One place I had previously noticed this same thing, is in W3ULEV https://github.com/NOAA-EMC/WW3/blob/f66b6d46ad8204a74ad4112ef39d5a230525f8c8/model/src/w3updtmd.F90#L2012 which expects A (3D) and VA (2D) https://github.com/NOAA-EMC/WW3/blob/f66b6d46ad8204a74ad4112ef39d5a230525f8c8/model/src/w3updtmd.F90#L2133 though in the call from W3WAVE, receives VA for both arguments https://github.com/NOAA-EMC/WW3/blob/f66b6d46ad8204a74ad4112ef39d5a230525f8c8/model/src/w3wavemd.F90#L1348 which I agree, it surprisingly seems to be OK with this!

JessicaMeixner-NOAA commented 7 months ago

I concur with everyone - consistency is great! I cannot think of anything else to consider here or a reason why this change would be bad.

MatthewMasarik-NOAA commented 7 months ago

Thanks for weighing in @JessicaMeixner-NOAA. Then all clear from me too. It's great you're taking this on @ukmo-ccbunney!

ukmo-ccbunney commented 7 months ago

Great- thanks for your input @MatthewMasarik-NOAA and @JessicaMeixner-NOAA . I'll get something started after the Easter break.