Allow the use of CopernicusMetadata to initialize sea ice
#644
Conversation
| is_three_dimensional(metadata::CopernicusMetadata) = | ||
| metadata.name == :temperature || | ||
| metadata.name == :salinity || | ||
| metadata.name == :u_velocity || | ||
| metadata.name == :v_velocity |
There was a problem hiding this comment.
I think you should blacklist the 2D variables rather than whitelisting 3D variables, since most variables are 3D
There was a problem hiding this comment.
Here I was trying to follow the design pattern here:
There was a problem hiding this comment.
You can achieve this by using a ! conditional because is_three_dimensional == !is_two_dimensional
There was a problem hiding this comment.
for example:
is_three_dimensional(metadata::CopernicusMetadata) =
!(metadata.name == :ice_concentration ||
metadata.name == :ice_thickness)
| if !is_three_dimensional(metadata) | ||
| return (0, 1) |
There was a problem hiding this comment.
Would it be better to build a Flat grid for 2D variables? Why do the z_interfaces matter?
There was a problem hiding this comment.
this line seems to want to take something for z, I tried returning nothing but I guess it didn't work
There was a problem hiding this comment.
yeah, the size and the halo must be 2-tuples to use nothing in z.
src/DataWrangling/metadata_field.jl
Outdated
| halo = (3, 3, 3), | ||
| cache_inpainted_data = true) | ||
| cache_inpainted_data = true, | ||
| fill_nans = nothing) |
There was a problem hiding this comment.
why not just use true or false here? Also I am confused --- shouldn't this be handled by inpainting? Inpainting is generally the process of removing invalid entries. Usually we inpaint NaNs with fancy propagation method, but just replacing them with a neutral value (like 0) could be implemented as an additional inpainting option.
There was a problem hiding this comment.
I suppose you're right, we should do it via inpainting. Currently there is no zero inpainting available, so I suppose the solution is to write a zero inpainting method and then change default_inpainting so that it deals with CopernicusMetadata differently? It seems like for ECCO no inpainting is required for sea ice
There was a problem hiding this comment.
Yes, the purpose of the default_inpainting is to allow dispatch on metadata type
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
==========================================
- Coverage 23.04% 22.96% -0.08%
==========================================
Files 48 48
Lines 2990 3009 +19
==========================================
+ Hits 689 691 +2
- Misses 2301 2318 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #643