Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dimensions in wave structure #1252

Merged
merged 13 commits into from
Dec 21, 2020

Conversation

raphaeldussin
Copy link
Contributor

routine was failing dimensional testing. Proposed fixes makes it dimensionally consistent.
I also add several dimension checks, available with DEBUG=true

I think the normalization of the vertical structure was wrong and I corrected accordingly.

In spite of the various fixes, tridiagonal solver becomes unstable at step=2 in benchmark :(

@raphaeldussin
Copy link
Contributor Author

@marshallward how do I figure out where the style problem comes from?

@codecov-io
Copy link

codecov-io commented Nov 19, 2020

Codecov Report

Merging #1252 (42758d1) into dev/gfdl (f3483be) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           dev/gfdl    #1252      +/-   ##
============================================
- Coverage     45.90%   45.88%   -0.02%     
============================================
  Files           225      225              
  Lines         71395    71422      +27     
============================================
  Hits          32774    32774              
- Misses        38621    38648      +27     
Impacted Files Coverage Δ
src/ALE/regrid_edge_values.F90 34.57% <ø> (ø)
src/diagnostics/MOM_wave_structure.F90 0.00% <0.00%> (ø)
...c/parameterizations/lateral/MOM_internal_tides.F90 0.00% <0.00%> (ø)
...meterizations/vertical/MOM_internal_tide_input.F90 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3483be...42758d1. Read the comment docs.

@adcroft
Copy link
Collaborator

adcroft commented Nov 20, 2020

@marshallward how do I figure out where the style problem comes from?

Evidently I messed up the error reporting when merging the two workflows. It was easier when they were separate. I can fix the reporting once the workshop is over...

@raphaeldussin
Copy link
Contributor Author

@adcroft it was a dumb whitespace error, I fixed it. It would be helpful though to have github actions print the styling errors found

@adcroft
Copy link
Collaborator

adcroft commented Nov 20, 2020

@adcroft it was a dumb whitespace error, I fixed it. It would be helpful though to have github actions print the styling errors found

Yes, and it used to. While developing the workflows I initially had separate workflows for style and docs but we decided we needed fewer workflows. In order to avoid a white space problem not block the testing of docs I had to do a workaround. I got it wrong.

Copy link
Collaborator

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

Overall, I think that there is progress with these changes, and suspect that they will all be found to be correct when used with an appropriate test case. However, it appears that there are new definitions for the rescaled units of a number of the variables in this routine, but there have not been any changes to the comments describing these variables to reflect these changes.

Please add comments describing the (now correct) units of the variables when the variables are defined. I am aware that some of the variables are not described in comments at all, but this should be the right opportunity to start doing so.

@raphaeldussin raphaeldussin marked this pull request as draft November 23, 2020 15:51
@raphaeldussin
Copy link
Contributor Author

I don't know how to fix style if I can't see the errors in the checks

@marshallward
Copy link
Collaborator

Looks like one of your lines is exceeding the 120-character limit due to the comment:

$ ./.testing/trailer.py -e TEOS10 -l 120 src config_src | tee style_errors
src/diagnostics/MOM_wave_structure.F90, line 166: Line length exceeded "  real, dimension(SZK_(G)+1) :: Uavg_profile !< Vertical profile of the magnitude of horizontal velocity [L T-1 ~> m s-1]."

We need to clean this up to give better feedback, I think.

@raphaeldussin
Copy link
Contributor Author

@marshallward this would be very helpful thanks!

@raphaeldussin raphaeldussin marked this pull request as ready for review December 14, 2020 23:28
@raphaeldussin
Copy link
Contributor Author

I think this is ready but I'd like to give @sonyalegg a chance to chime in

@adcroft
Copy link
Collaborator

adcroft commented Dec 19, 2020

@marshallward marshallward merged commit b572249 into mom-ocean:dev/gfdl Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants