Skip to content

fix call to tri-diagonal solver in MOM_wave_structure.#154

Merged
marshallward merged 3 commits into
NOAA-GFDL:dev/gfdlfrom
raphaeldussin:fix_call_tridiag_wavestruct
Jul 11, 2022
Merged

fix call to tri-diagonal solver in MOM_wave_structure.#154
marshallward merged 3 commits into
NOAA-GFDL:dev/gfdlfrom
raphaeldussin:fix_call_tridiag_wavestruct

Conversation

@raphaeldussin
Copy link
Copy Markdown

The call to the solve_diag_dominant_tridiag had arguments mixed-up:

subroutine solve_diag_dominant_tridiag( Al, Ac, Au, R, X, N )

lower diagonal: c_diag = Al in the function prototype
midlle diagonal: b_diag = Ad NOT in the function prototype
upper diagonal: a_diag = Au in the function prototype

the function call for Ac of the middle diagonal, defined as

Ad = Ac+Al+Au

Ac=Ad-Al-Au

so that the correct argument for Ac is b_diag - a_diag - c_diag = b_diag - (a_diag + c_diag)

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 21, 2022

Codecov Report

Merging #154 (335a5d9) into dev/gfdl (c9dd804) will not change coverage.
The diff coverage is n/a.

❗ Current head 335a5d9 differs from pull request most recent head 6940115. Consider uploading reports for the commit 6940115 to get more accurate results

@@            Coverage Diff            @@
##           dev/gfdl     #154   +/-   ##
=========================================
  Coverage     34.02%   34.02%           
=========================================
  Files           259      259           
  Lines         70204    70204           
  Branches      13010    13013    +3     
=========================================
  Hits          23884    23884           
  Misses        41822    41822           
  Partials       4498     4498           
Impacted Files Coverage Δ
src/diagnostics/MOM_wave_structure.F90 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 c9dd804...6940115. Read the comment docs.

@Hallberg-NOAA Hallberg-NOAA self-assigned this Jun 21, 2022
Copy link
Copy Markdown
Member

@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.

I agree that this change corrects an error in the arguments to the (altered) tridiagonal solver that was introduced with
mom-ocean@7062289 .

The altered code is technically correct as written. However, the MOM6 style guide at https://github.com/NOAA-GFDL/MOM6/wiki/Code-style-guide#array-syntax does not encourage the use array-syntax math in subroutine arguments. Given that b_diag is not used anywhere else, apart from in this subroutine call, perhaps it would make sense to define a new variable that is this argument, and to replace the expressions setting b_diag (or perhaps leave the expressions for b_diag in comments for clarity).

@marshallward
Copy link
Copy Markdown
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16102 ✔️

It was decided that the issues around array syntax are no worse than the existing form, and can be addressed in a later PR.

@marshallward marshallward merged commit 58d704b into NOAA-GFDL:dev/gfdl Jul 11, 2022
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.

3 participants