-
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
Allow users to mask output #3092
Open
navidcy
wants to merge
23
commits into
main
Choose a base branch
from
ncc/mask-output
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
a59560c
add mask_value in outputwriters + mask in fetch_output
navidcy 1089660
mask_immersed_field! -> mask_immersed!
navidcy 32f3afe
add mask_immersed kwargs in ow
navidcy 4e2c1b9
no need to explicitly mask immersed
navidcy 903df9b
fix bugs
navidcy f26e2f0
remove stray ζ
navidcy 1500861
don't mask immersed by default
navidcy 54fa67f
add mask_immersed kwarg
navidcy 19c3679
better fetch_output
navidcy 9d796b0
is -> isa
navidcy 067998e
Update fetch_output.jl
glwagner 5a24fb7
mask_immersed_field -> mask_immersed
navidcy c8efbcb
Merge branch 'ncc/mask-output' of github.com:CliMA/Oceananigans.jl in…
navidcy 4131ff0
mask_immersed_field -> mask_immersed
navidcy 1519aa0
Merge branch 'main' into ncc/mask-output
tomchor 64404a8
Merge branch 'main' into ncc/mask-output
navidcy d45cfd6
remove wait comments
navidcy e5872a0
some updates
navidcy 9ae2919
remove empty line
navidcy e716bf0
remove empty line
navidcy d76ae85
Merge branch 'main' into ncc/mask-output
navidcy 7a4873d
Merge branch 'main' into ncc/mask-output
tomchor cbd54b8
Merge branch 'main' into ncc/mask-output
navidcy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
using KernelAbstractions: @kernel, @index | ||
using Statistics | ||
using Oceananigans.Architectures: architecture | ||
using Oceananigans.Fields: location, ZReducedField, Field | ||
|
||
instantiate(T::Type) = T() | ||
instantiate(t) = t | ||
|
||
mask_immersed!(field, grid, loc, value) = nothing | ||
mask_immersed!(field::Field, value=zero(eltype(field.grid))) = | ||
mask_immersed!(field, field.grid, location(field), value) | ||
|
||
""" | ||
mask_immersed!(field::Field, grid::ImmersedBoundaryGrid, loc, value) | ||
|
||
Mask `field` defined on `grid` with a `value` at `loc`ations at grid points where [`peripheral_node`](@ref) | ||
is to `true`. | ||
""" | ||
function mask_immersed!(field::Field, grid::ImmersedBoundaryGrid, loc, value) | ||
arch = architecture(field) | ||
loc = instantiate.(loc) | ||
launch!(arch, grid, :xyz, _mask_immersed!, field, loc, grid, value) | ||
return nothing | ||
end | ||
|
||
|
||
@kernel function _mask_immersed!(field, loc, grid, value) | ||
i, j, k = @index(Global, NTuple) | ||
@inbounds field[i, j, k] = scalar_mask(i, j, k, grid, grid.immersed_boundary, loc..., value, field) | ||
end | ||
|
||
mask_immersed_horizontal!(field, args...; kw...) = nothing | ||
mask_immersed_horizontal!(::Nothing, args...; kw...) = nothing | ||
mask_immersed_horizontal!(field, value=zero(eltype(field.grid)); k, mask = peripheral_node) = | ||
mask_immersed_horizontal!(field, field.grid, location(field), value; k, mask) | ||
|
||
""" | ||
mask_immersed_horizontal!(field::Field, grid::ImmersedBoundaryGrid, loc, value; k, mask) | ||
|
||
Mask `field` on `grid` with a `value` on the slices `[:, :, k]` where `mask` is `true`. | ||
""" | ||
function mask_immersed_horizontal!(field::Field, grid::ImmersedBoundaryGrid, loc, value; k, mask) | ||
arch = architecture(field) | ||
loc = instantiate.(loc) | ||
return launch!(arch, grid, :xy, _mask_immersed_horizontal!, field, loc, grid, value, k, mask) | ||
end | ||
|
||
@kernel function _mask_immersed_horizontal!(field, loc, grid, value, k, mask) | ||
i, j = @index(Global, NTuple) | ||
@inbounds field[i, j, k] = scalar_mask(i, j, k, grid, grid.immersed_boundary, loc..., value, field, mask) | ||
end | ||
|
||
##### | ||
##### Masking for GridFittedBoundary | ||
##### | ||
|
||
@inline scalar_mask(i, j, k, grid, ::AbstractGridFittedBoundary, LX, LY, LZ, value, field, mask=peripheral_node) = | ||
@inbounds ifelse(mask(i, j, k, grid, LX, LY, LZ), value, field[i, j, k]) |
This file was deleted.
Oops, something went wrong.
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
@glwagner I think this won't work because what happens is that, e.g., if
u
is the output to be saved thenmodel.velocities.u
is masked and then it includes NaNs at the next step so the whole state NaN...At least that's what I found happening and I boil it down to this line here masking and then returning the parent of the field. Am I right?
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.
Ok that makes sense. But the real problem is how we define masking. Consider: the mask we apply to a field for output should not affect time-stepping. If it does, there's a problem. So let's fix that before hacking around it.
The issue is velocity components I think. The "mask" is defined to cover the points that are on the boundary, not just inside it. Thus masking the velocity to 0 is how we ensure impenetrability.
The confusion is therefore: are velocity nodes on the boundary immersed or not?
I think we should probably define this points as not immersed. Therefore to fix the problem we can:
inactive_node
andperipheral_node
can be used for this).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.
Yes, I was thinking of point 2 you make and modified
masked_immersed!
to useinactive_node
(just to test the hypothesis) but that didn't work either.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.
There might also be a problem with the implicit solver to fix. Our strategy right now is to zero out elements of the matrix that correspond to immersed cells. Unfortunately
0 * NaN = NaN
so anyNaN
in the immersed boundary will be propagated immediately in the whole columnThere 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.
Good point.
Can't we just apply the mask to the saved output?
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.
Oh that "implicit solver".
I guess that is the code here:
Oceananigans.jl/src/ImmersedBoundaries/grid_fitted_immersed_boundaries.jl
Lines 126 to 148 in a1f4f4e
Seems like we would need
Oceananigans.jl/src/Solvers/batched_tridiagonal_solver.jl
Line 179 in a1f4f4e
to return 0 for
immersed_cell
.To accomplish this without explicit masking we might have to wrap
field
in aConditionalOperation
:Oceananigans.jl/src/TurbulenceClosures/vertically_implicit_diffusion_solver.jl
Lines 197 to 199 in a1f4f4e
hmm...
Another possibility is to move
mask_immersed!
fromupdate_state!
; for example if we masked fields beforeab2_step!
then I think we'd be ok:Oceananigans.jl/src/TimeSteppers/quasi_adams_bashforth_2.jl
Line 93 in a1f4f4e
Not sure if that would affect non-masked output. Hopefully not.
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.
I mean only mask the data written in the jld2/nc file, not the actual data in the u, v, w, fields, etc.
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.
Data has to be written from an existing array. So, masking data being saved to disk without touching the "actual" fields requires allocating a temporary array. Note, in the GPU case we already do this on the CPU; however we would then have to mask on the CPU which could slow things down for large fields.
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.
OK, I see!
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.
But maybe what you're suggesting could be achieved by digging into JLD2's array-saving code. I'm not sure about NetCDF. I took a quick look and couldn't figure it out but such masking during i/o might be possible...