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

User interface for masking output on ImmersedBoundaryGrid #3061

Open
glwagner opened this issue Apr 12, 2023 · 16 comments · May be fixed by #3092
Open

User interface for masking output on ImmersedBoundaryGrid #3061

glwagner opened this issue Apr 12, 2023 · 16 comments · May be fixed by #3092

Comments

@glwagner
Copy link
Member

I have found it inconvenient that output is masked to 0. For most purposes, I'd prefer NaN. I'm opening this issue to discuss a user interface that would implement such a feature.

My first idea is to add a property / kwarg to output writers called immersed_values.

Then we might write something like

output_writer = JLD2OutputWriter(model, output, immersed_values=NaN, other_kwargs...)

The default will be immersed_values=nothing. Then fetch_output:

function fetch_output(field::AbstractField, model)
compute_at!(field, time(model))
return parent(field)
end

would become something like

 function fetch_output(field::AbstractField, model, immersed_value) 
    compute_at!(field, time(model))
    !isnothing(immersed_value) && mask_immersed_field!(field, immersed_value)
    return parent(field) 
 end 
@glwagner
Copy link
Member Author

@tomchor

@navidcy
Copy link
Collaborator

navidcy commented Apr 28, 2023

Yeah, let's do that! I'll open a PR

@glwagner
Copy link
Member Author

What kwarg name will you use?

@glwagner
Copy link
Member Author

Let’s discuss the user API here though it’s pretty simple

@navidcy
Copy link
Collaborator

navidcy commented Apr 28, 2023

you mean what kwarg for the JLD2/NetCDFOutputWriter? perhaps replace_land_with_NaN = true ?

@glwagner
Copy link
Member Author

Mm yeah that’s an alternative! I think we could allow any number. As for “land” versus “immersed” I guess land is a little more oceany but elsewhere we talk about immersed stufff…

@glwagner
Copy link
Member Author

Or immersed_mask = 0?

@glwagner
Copy link
Member Author

The concept of “fill” or “replace” might be helpful. Like “fill_immersed=0”

@simone-silvestri
Copy link
Collaborator

I like the concept of mask, what about immersed_mask_value=0?

@glwagner
Copy link
Member Author

glwagner commented Apr 28, 2023

I think that's a good one. So we have:

  • immersed_value = 0
  • immersed_mask_value = 0
  • mask_immersed = 0
  • fill_immersed = 0

Would even just mask = 0 be sufficient? I've also been annoyed about "mask_immersed_field" and wondered whether simply mask!(field) would be clear and a little cleaner

@navidcy
Copy link
Collaborator

navidcy commented Apr 28, 2023

I vote for simply mask = default_value and mask!(field)

@navidcy
Copy link
Collaborator

navidcy commented Apr 28, 2023

The mask_value should be added as a field of each writer type, right?

@glwagner
Copy link
Member Author

yes, it has to be a property of JLD2OutputWriter and NetCDFOutputWriter

@tomchor
Copy link
Collaborator

tomchor commented Apr 28, 2023

I think it's important to have the word immersed somewhere in the flag name otherwise it might not be clear that we're masking. I vote for immersed _mask_value flag. It's kinda verbose, but imo 100% clear.

@navidcy
Copy link
Collaborator

navidcy commented Apr 28, 2023

mask_immersed = 0

kwarg in the output writers and

mask_immersed!(field)

?

@glwagner
Copy link
Member Author

I like it! The concept a function name that is something like take_action_on(argument) is typical, eg the function fill!(array, value) rather than fill_value!(array, value) (which is redundant for the reader). I liek the readability of mask_immersed = 0, as in "mask immersed [grid cells with] = value".

@navidcy navidcy linked a pull request Apr 30, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants