+Move wave module variables into control structure#1404
Merged
Conversation
Moved 15 module variables into wave_parameters_CS, to follow the coding style and conventions in the rest of the code. Also added comments describing 8 enumeration parameters in this same module, and added comments suggesting ways that the accuracy of the calculations could be improved. Also incorporated a factor of 2*PI into the definition of Freq_Cen in this module. In addition, the initialization procedure for this module was altered in some more commonly used cases, so that use MOM_wave_interface_init to set wave parameters, even with a statistical wave model, and eliminated MOM_wave_interface_init_lite. I believe that all solutions should be bitwise identical, but this module is not yet well tested in the MOM6 regression tests.
breichl
approved these changes
May 19, 2021
Collaborator
breichl
left a comment
breichl
left a comment
There was a problem hiding this comment.
Thanks @Hallberg-NOAA, these look like good changes and improvements to the code. As you point out, the testing is not presently sufficient and this will be addressed soon, but I've checked some cases offline and it looks to not change any functionality. I approve. I made just a couple of minor comments on comments, but nothing that actually changes the code.
Corrected typos in comments, and removed unnecessary commented out code as noted by Brandon Reichl in his review of MOM6 dev/gfdl PR# 1404. All answers are bitwise identical.
Collaborator
Author
|
Brandon Reichl has approved these changes. He will be following this up with further changes to eliminate some of the redundant options. |
Collaborator
marshallward
approved these changes
May 26, 2021
Collaborator
marshallward
left a comment
marshallward
left a comment
There was a problem hiding this comment.
Approving on behalf of @breichl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moved 15 module variables into wave_parameters_CS, to follow the coding style
and conventions in the rest of the code. Also added comments describing 8
enumeration parameters in this same module, and added comments suggesting ways
that the accuracy of the calculations could be improved. Also incorporated a
factor of 2*PI into the definition of Freq_Cen in this module. In addition, the
initialization procedure for this module was altered in some more commonly used
cases, so that use MOM_wave_interface_init to set wave parameters, even with a
statistical wave model, and eliminated MOM_wave_interface_init_lite. I believe
that all solutions should be bitwise identical, but this module is not yet well
tested in the MOM6 regression tests. The commits in this PR include: