-
Notifications
You must be signed in to change notification settings - Fork 191
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 typo in application of y value BCs #690
Conversation
Codecov Report
@@ Coverage Diff @@
## master #690 +/- ##
==========================================
+ Coverage 78.05% 78.18% +0.12%
==========================================
Files 120 120
Lines 2447 2443 -4
==========================================
Hits 1910 1910
+ Misses 537 533 -4
Continue to review full report at Codecov.
|
Hmm... I don’t think this is a typo. The functions *_gradient for “west”, “south”, and “bottom” are identical. Therefore we should use the alias “left” for all three to avoid copy-pasting identical functions. The same applies to the three “right” boundaries: “east”, “north”, and “top”. If we want to use matching names in other functions then we should use ‘const’ aliases. I think copy-pasting function definitions is bad practice that decreases maintainability and can make bugs harder to catch and fix. |
This is not part of this PR, but these 4 functions have identical definitions: @inline bottom_gradient(bc::GBC, c¹, Δ, i, j, args...) = getbc(bc, i, j, args...)
@inline top_gradient(bc::GBC, cᴺ, Δ, i, j, args...) = getbc(bc, i, j, args...)
@inline south_gradient(bc::GBC, c¹, Δ, i, k, args...) = getbc(bc, i, k, args...)
@inline north_gradient(bc::GBC, cᴺ, Δ, i, k, args...) = getbc(bc, i, k, args...) |
@@ -36,8 +36,8 @@ end | |||
@inline bottom_gradient(bc::VBC, c¹, Δ, i, j, args...) = ( c¹ - getbc(bc, i, j, args...) ) / (Δ/2) | |||
@inline top_gradient(bc::VBC, cᴺ, Δ, i, j, args...) = ( getbc(bc, i, j, args...) - cᴺ ) / (Δ/2) | |||
|
|||
@inline left_gradient(bc::VBC, c¹, Δ, i, k, args...) = ( c¹ - getbc(bc, i, k, args...) ) / (Δ/2) | |||
@inline right_gradient(bc::VBC, cᴺ, Δ, i, k, args...) = ( getbc(bc, i, k, args...) - cᴺ ) / (Δ/2) | |||
@inline south_gradient(bc::VBC, c¹, Δ, i, k, args...) = ( c¹ - getbc(bc, i, k, args...) ) / (Δ/2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function's definition is identical to bottom_gradient
.
I think we should have two functions, left_gradient
and right_gradient
. left_gradient
should be called in _fill_bottom_halo!(c, bc::Union{VBC, GBC}, grid, args...)
, _fill_south_halo!(c, bc::Union{VBC, GBC}, grid, args...)
, and _fill_west_halo!(c, bc::Union{VBC, GBC}, grid, args...)
, while right_gradient
should be called in the same functions for top
, north
, and east
.
Good observation. I've simplified the code down to just |
No description provided.