Skip to content

Increase valid_max for f in ounp#1014

Merged
MatthewMasarik-NOAA merged 3 commits into
NOAA-EMC:developfrom
eccc-waves:bugfix/988-valid_max_too_low
May 31, 2023
Merged

Increase valid_max for f in ounp#1014
MatthewMasarik-NOAA merged 3 commits into
NOAA-EMC:developfrom
eccc-waves:bugfix/988-valid_max_too_low

Conversation

@benoitp-cmc
Copy link
Copy Markdown
Contributor

@benoitp-cmc benoitp-cmc commented May 23, 2023 •
edited
Loading

Pull Request Summary

The valid_max attribute for the f variable (1D spectra energy) in ww3_ounp is set to 100. It can exceed that value.

Description

Set the valid_max attribute of the f variable to 1e20. Otherwise, large energy gets masked out (it occured with hurricane Fiona 2022).

Please also include the following information:

  • Add any suggestions for a reviewer
  • Mention any labels that should be added: bug
  • Are answer changes expected from this PR? Only for NetCDF output (ounp) with POINT%TYPE = 1; SPECTRA%OUTPUT = 2.

Issue(s) addressed

Commit Message

Increase valid_max for f in ounp #988

Check list

Testing

  • How were these changes tested? Regression tests and high energy wave case (https://hpfx.collab.science.gc.ca/~bpo001/WW3/issue_988/)
  • Are the changes covered by regression tests? No. No ounp output with ITYPE=1 and OTYPE=2. Output could be activated in some test.
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)? Yes, ECCC HPC, intel.
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.) Differences as expected I think
  • Please
    matrixCompSummary.txt
    matrixDiff.txt
    provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):
    matrixCompFull.txt

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 24, 2023

@benoitp-cmc, this sounds like a good find. I'm curious about the new value you selected for valid_max. You mentioned in #988 a Fiona case where the values got near 300, is there a reason you choose a new value as high as 1E20?

Fyi, @ukmo-ccbunney @mickaelaccensi

@benoitp-cmc
Copy link
Copy Markdown
Contributor Author

benoitp-cmc commented May 24, 2023

  • The max value for the 2D spectra is 1E20, so this has the advantage of being coherent.
  • Selecting a lower max value doesn't seem to have an impact on file size.
  • Selecting a realistic max is tricky. It's probable that 1000 is high enough in real Earth conditions. If not 1e20, I'd be tempted to go with 10 000 to be on the safe side and to allow testing in more extreme conditions.

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 24, 2023

  • Selecting a realistic max is tricky. It's probable that 1000 is high enough in real Earth conditions. If not 1e20, I'd be tempted to go with 10 000 to be on the safe side and to allow testing in more extreme conditions.

@benoitp-cmc, thank you for your explanation. I can see how selecting 1e20 is attractive if it is tricky to pick a realistic max value. I'm curious if others have thoughts on the value to use (going with 1E20, or trying to find a realistic valid_max)?

@ukmo-ccbunney
Copy link
Copy Markdown
Collaborator

ukmo-ccbunney commented May 24, 2023

100 is certainly to low!

I would say that 1e20 is a bit excessive, even if it is in line with the max value for the 2D spectra (I couldn't immediately see where this was set).

To me 1000 seems like a sensible value - I've never heard of real world energy densities anywhere near this, but could be wrong!

Equally, 10000 would be fine - it does give some leeway in case someone wants to do something crazy like mess around with hypothetical waves in high gravity environments!

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 24, 2023

@ukmo-ccbunney thanks for adding your perspective. I was kind of ballparking the same (1000, 10000) for realistic max values, if we want to go with realistic, which I'm not saying we need to. Maybe we'll wait a bit to see if anyone else wants to chime in? I don't have a real strong preference for a particular choice at this point

@mickaelaccensi
Copy link
Copy Markdown
Collaborator

mickaelaccensi commented May 25, 2023

I think 1e20 comes from efth's valid max which is due to its scale factor at 0.0004. So here, for f variable it should not be that, it is probably a copy/paste...

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 25, 2023

I think 1e20 comes from efth's valid max which is due to its scale factor at 0.0004. So here, for f variable it should not be that, it is probably a copy/paste...

@mickaelaccensi thank you for this insight. @benoitp-cmc, you mentioned 1000 or 10 000 for realistic values. Does one of those or a different realistic value seem acceptable to you, here?

@mickaelaccensi
Copy link
Copy Markdown
Collaborator

mickaelaccensi commented May 25, 2023

1000 sounds ok

@benoitp-cmc
Copy link
Copy Markdown
Contributor Author

benoitp-cmc commented May 25, 2023

@MatthewMasarik-NOAA I agree on 1000. A maximum that is extremely unlikely to be exceeded in normal conditions is probably more useful than one that would just be useful in theoretical studies.

If someone is doing waves on exoplanet using this model, they probably have many other things to adjust.

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 25, 2023

@MatthewMasarik-NOAA I agree on 1000. A maximum that is extremely unlikely to be exceeded in normal conditions is probably more useful than one that would just be useful in theoretical studies.

If someone is doing waves on exoplanet using this model, they probably have many other things to adjust.

Great. Thanks, @benoitp-cmc. I'll run the test you provided, then continue with the approval process if that checks out. Thanks @ukmo-ccbunney and @mickaelaccensi for your input.

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 26, 2023

@benoitp-cmc I wanted to run the test case you listed at: https://hpfx.collab.science.gc.ca/~bpo001/WW3/issue_988/. Can you also provide the switch file you compiled with?

@benoitp-cmc
Copy link
Copy Markdown
Contributor Author

benoitp-cmc commented May 30, 2023

@MatthewMasarik-NOAA I should have thought about this. I added the switch with the rest: https://hpfx.collab.science.gc.ca/~bpo001/WW3/issue_988/switch

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 30, 2023

@MatthewMasarik-NOAA I should have thought about this. I added the switch with the rest: https://hpfx.collab.science.gc.ca/~bpo001/WW3/issue_988/switch

@benoitp-cmc perfect, thanks. I'll get this tested.

MatthewMasarik-NOAA
MatthewMasarik-NOAA approved these changes May 31, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MatthewMasarik-NOAA MatthewMasarik-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review PASS

Testing PASS

  • Regression tests were not run in this case, only the program ww3_ounp was modified and was tested with the test case provided for it.
  • The ww3_ounp test case was run with the current develop branch, and seen to display a message in ncview when the data (304.252) exceeded the valid_max value (100) for f.
pr_1014_dev
  • The test case was then run with the 'fix' branch and the maximum value is seen to be 304.252 in ncview.
pr_1014_fix

@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 31, 2023

Hi @benoitp-cmc, I just approved your PR. Could you sync your branch up and I will merge it?

@benoitp-cmc
Copy link
Copy Markdown
Contributor Author

benoitp-cmc commented May 31, 2023

@MatthewMasarik-NOAA done. Thanks!

@MatthewMasarik-NOAA MatthewMasarik-NOAA merged commit 80572c5 into NOAA-EMC:develop May 31, 2023
@MatthewMasarik-NOAA
Copy link
Copy Markdown
Contributor

MatthewMasarik-NOAA commented May 31, 2023

Thank you for this fix to ww3_ounp, @benoitp-cmc!

@benoitp-cmc benoitp-cmc deleted the bugfix/988-valid_max_too_low branch May 31, 2023 18:21
MatthewMasarik-NOAA added a commit to MatthewMasarik-NOAA/WW3 that referenced this pull request Jun 27, 2023
@MatthewMasarik-NOAA
Merge remote-tracking branch 'origin/develop' into syncdev2ufs_2023jun27
0c26878 
* origin/develop:
  handle NaN air-sea temperatures from nearest land points (NOAA-EMC#869)
  Increase valid_max for f in ounp (NOAA-EMC#1014)
  Bugfix deallocation of invalid memory in ww3_prnc (NOAA-EMC#1016)
  Update to orion intel module path and two typo corrections. (NOAA-EMC#1011)
  Bugfix to out of bounds array write in w3profsmd_pdlib.f90 (NOAA-EMC#1013)
  Simple logic fix for time interpolation of boundary nodes at the end of W3XYPFSNIMP. (NOAA-EMC#1005)
  in w3iors use NSEA instead of NSEAL in serial write/read of VA (NOAA-EMC#954)
  Update documentation for UNST namelist (NOAA-EMC#986)
  In certain coupled configurations, the piece of code testing the coupling frequency to check if 'receive' coupling exchanges need to take place fail, resulting in an infinite loop causing the integration between time zero and the first time step to repeat indefinitely. This check needs to be rewritten, which fixes also issue NOAA-EMC#816 in a simpler way.  (NOAA-EMC#999)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@MatthewMasarik-NOAA MatthewMasarik-NOAA MatthewMasarik-NOAA approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"valid_max" too low for "f" variable in ww3_ounp

4 participants

@benoitp-cmc @MatthewMasarik-NOAA @ukmo-ccbunney @mickaelaccensi