Bugfix/negative peak frequency #741
Conversation
…alculation in ww3_outp
…ogomd, ww3_outp and ww3_ounp (NOAA-EMC#304)
|
@benoitp-cmc I don't see where an IS2 ifdef is added? |
|
@JessicaMeixner-NOAA it turned out a bit strange in the rebase. I had added |
|
@benoitp-cmc I've run the full set of regression tests and we have a lot of changes: matrixCompFull.txt Is this expected? Can you provide some additional information about how this PR was tested? |
|
@JessicaMeixner-NOAA thank you. It affects cases at both end of the frequency range.
We've been running in operation with this patch for about a year now. Before implementing it I had a few runs where I could reproduce the low frequency bug, and wher I added extra output at points in the neighbourhood to see exactly what was going on and see if the result made sense. I would have to dig it back. |
|
The issue here is that we're changing so many regression test answers and based on your explanation of the code change, I'd expect a rare change, not a lot of changes. So either a lot of the regression tests are triggering these rare cases or there's an unintentional change that needs to be diagnosed further. |
This does not surprise me much. The high frequency difference is not rare, though each time I expect it to affect only a few points and to be small. We probably would need to create a new regression test to catch the low frequency case.
|
|
@benoitp-cmc I have run the regressions tests and I see lots of difference as Jessica did (in the out_grd files, etc) - as might be expected. However, I notice that in some cases where You can see this behaviour for example in the netCDF outputs for |
|
@ukmo-ccbunney There is a check I guess if IKP0=NK, then I'll amend the code and I'll try my hand at running regression tests, at least |
|
Thanks @ukmo-ccbunney that was a good catch. By forbidding the highest frequency (as before), the bulk of what was UNDEF before is UNDEF again. There are differences though because the lowest frequency (IKP0=1) is now allowed. Looking at
Before PR After PR Using a log scale. Sorry for the inconsistent appearance, the minimum of the before is lower (0.031) than after (0.038). The difference are all in the rows near -5.5E. Rerunning regtests now. |
Regression test differencesI've went through many of the regression tests. In the table below is a summary of my observations. The peak frequencies that were out of bound are marked in bold.
Peak direction difference in
|
|
@benoitp-cmc Thanks for doing this great work and for providing extensive analysis on the comments provided by @ukmo-ccbunney @JessicaMeixner-NOAA. I am going to check at my end and will come back to you. |
|
@benoitp-cmc I have taken over the review and testing of this PR. All the regression tests pass, but as there are many changes I'm going through carefully to ensure that they are only the intended changes and nothing unexpected. I suspect this might take me a few days, but I hope to get this merged next week. Thank you so much for your patience and updates in response to comments. |
|
@benoitp-cmc just a quick update to let you know I'm still going through this PR and checking all of the output for the differences that I'm seeing. Thanks again for your patience as this is taking longer than expected. |
|
@JessicaMeixner-NOAA I feel your pain, it is long and tedious to go through the reg tests. I'm happy as long as it's resolved before the next major release. |
|
@benoitp-cmc thank you so much for your updates and your patience through the review process. I can confirm that I get the same output as you and these code changes are good to go. More details: The matrix output (diff was too large to include): Looking through the diffs, they are all related to peak frequency, and as @benoitp-cmc has indicated earlier in the thread these are 'good' differences. To make sure all of the regression tests were as expected, I ended up running both this branch and the develop branch with additional output and also seperated the netcdf files by variable to help confirm that the changes were what was expected. That branch with extra output for the develop branch can be found here: https://github.com/JessicaMeixner-NOAA/WW3/tree/extraoutput for anyone who is interested, and the output of the comparison can then be seen here: matrixCompFull.txt I'm running one additional test so I can include here whether this will change answers for another system. I'm anticipating to merge this PR later today though. Thanks again @benoitp-cmc |
|
@JessicaMeixner-NOAA thanks so much for taking the time to go through this. Great idea to split the output. |
|
@benoitp-cmc my one additional test was our coupled model regression tests here at EMC. The debug test there is failing on: I'm going to see if I can recreate this issue in one of the standalone WW3 regression tests which will be easier to debug. I'm hoping this is a pretty quick fix, but if it's okay with you I'm going to try to figure out this issue before merging. The other option would be to merge this and then immediately put in a fix, but I think it might be best to just to try to fix this now? Thoughts? |
|
@JessicaMeixner-NOAA agreed, this should be fixed before merging. Strange. I don't see anywhere where the first index of EBD could be 0. We do allow IKP0=1 now (before was 2), but as far as I can tell all the EBD are protected. |
|
I started the set of matrix regtests with debug compile on and am also working on just looking at our coupled test to see if I can't figure it out from there. I'll keep you up to date with what I find. |
|
Okay so if I update run with debug on, here are some of the regtests that give me an error: I'm thinking it's when we call EBD(IK-1, *) such as: Here's so far the line numbers where I get errors from: @benoitp-cmc Let me know what your thoughts are, I can help with code updates, additional tests, etc. |
|
I see, I had added the That explains 2163, 2147, 2155. I have no idea why 1507, 1498 would give errors, that would be unrelated. I did not want to touch FP1. Since the allowable range of IKP1 is 2 to NK-1, there is no risk of the calculation degenerating. (We could simplify the calculation by using what I did for EL and EH for FP0 and forget the ILOW. We should also add a check on I am not sure how to do this the most cleanly.
|
|
I'd say let's try fixing the ones we know are related to this and then see if the other errors go away (they could have been from not the first MPI tasks for example during the error out or from completely unrelated errors as you mentioned). Admittedly, I was not thinking about the fact that these could be in other loops for different variables when I was just searching for this issue. My first thought is to just go with how you did other parts of the code you updated here, but if you think the other way is cleaner, then I have no objections. Calls to INIT_GET_ISEA aren't the best, but depending on how many if-statements you add it could be about equal. |
|
As far as I can tell, the FP1 calculated in @JessicaMeixner-NOAA I would suggest we remove FP1 and THP1 entirely. We would not touch |
|
@benoitp-cmc I've been looking in the code and I agree that the only time I see FP1 used, it's defined locally in w3src2, it does not look like FP1 and THP1 are used anywhere, in fact I see: in W3ADATMD. Removing these variables should be in its own PR so we can confirm there's no unintended effects. Would you have time to do that or do you need me to make that PR? In parallel, I will delete those lines and run the debug tests to confirm that solves the issue I encountered with this PR. I made an issue with this update: #803 |
|
Thanks. Make sense to have the removal in a separate pull request. I can take a stab at it tomorrow. |
|
@benoitp-cmc sounds good. Let me know what I can do to help. In the meantime, I put in the issue a potential problem with removal but I think we can work around it. |
|
@JessicaMeixner-NOAA when will you merge this? There might be some merge conflicts with the removal of FP1 and THP1. |
|
I don't want to merge this until we can fix the debug issues. So we'd either need to do a temporary fix here and then remove FP1/THP1 or remove FP1/THP1 first and then merge all of that here. If there are merge isssues, I'd just need to ask you to merge the develop branch into your branch with the PR here instead of just merging things locally when I'm running tests which is what I've been doing. |
|
@benoitp-cmc as expected, this branch now has conflicts that needs to be resolved after PR #807. Can you merge the develop branch into this branch and then I'll start testing this again, including the debug testing? |
|
Thanks @benoitp-cmc I'll start running tests! |
|
@benoitp-cmc just wanted to let you know that the debug test that previously was failing is now passing. Still re-running the full set of tests which might run into our machine having a 2 day maintenance, but things are looking good so far! |
|
Hi @benoitp-cmc , I wanted to give a short update. The machine maintenance finished yesterday, though @JessicaMeixner-NOAA is out of office today. She will be back Monday and we will work to finalize this PR then. Thank you for your patience. |
|
Same tests are different: caused by the changes in FP. Full list of diffs (minus diff file which was too big): For ufs, this changes answers, see log: |
|
@benoitp-cmc thank you so much for your great work and your patience with getting this merged! |
|
Thank you @JessicaMeixner-NOAA and everyone for reviewing and steering this in the right direction. |
Pull Request Summary
Fix peak frequency calculation in extreme cases. Changes in w3iogomd, ww3_outp and ww3_ounp.
Description
In some cases where there is very little energy in the spectrum, peak frequency can occur at the min or max frequency. Currently this leads to faulty extrapolation, in some cases we see negative peak frequency. This PR ensures that peak frequency is kept within the min/max frequency range.
Edit: The
FPrange is inclusive of min frequency and exclusive of max frequency. With this PR, the calculation of peak frequency is consistent between gridded and point output except for the use of HSMIN in the point output #311.While editing ww3_outp, I added an IS2 ifdef to some variable definition to prevent a crash.
Please also include the following information:
Issue(s) addressed
Commit Message
Fix peak frequency calculation in extreme cases.
Check list
Testing