Add Var.push_dim #354
Add Var.push_dim #354
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
=======================================
Coverage 99.44% 99.44%
=======================================
Files 18 18
Lines 1965 1978 +13
=======================================
+ Hits 1954 1967 +13
Misses 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ph-kev
force-pushed
the
kp/singleton
branch
4 times, most recently
from
b8809c7 to
3a8d485
Compare
| function push_dim(var::OutputVar, dim_name, dim_value, dim_attrib_pairs...;) | ||
| dims = deepcopy(var.dims) |
There was a problem hiding this comment.
This doesn't work properly if dim_value is a Dates.DateTime. I am not too sure how crucial that functionality is.
There was a problem hiding this comment.
Is this because dims is a dict with values that are arrays of floats? Could you just make a new dict and the loop over the old one and add the keys/values?
There was a problem hiding this comment.
If you don't plan on supporting this, it might be good to catch the case and give an informative error
| Base.permutedims(var::OutputVar, perm) | ||
| Var.push_dim | ||
| Var.reordered_as |
There was a problem hiding this comment.
I am also not sure if this belongs in ClimaAnalysis extension in ClimaCalibrate or ClimaAnalysis.
| function push_dim(var::OutputVar, dim_name, dim_value, dim_attrib_pairs...;) | ||
| dims = deepcopy(var.dims) |
There was a problem hiding this comment.
Is this because dims is a dict with values that are arrays of floats? Could you just make a new dict and the loop over the old one and add the keys/values?
| If you want to change the order of the dimensions, then call `permutedims` after | ||
| this function. | ||
| """ | ||
| function push_dim(var::OutputVar, dim_name, dim_value, dim_attrib_pairs...;) |
There was a problem hiding this comment.
I'm not a huge fan of this function name, but I also can't think of a better name. Maybe something like
append_singleton_dim?
| function push_dim(var::OutputVar, dim_name, dim_value, dim_attrib_pairs...;) | ||
| dims = deepcopy(var.dims) |
There was a problem hiding this comment.
If you don't plan on supporting this, it might be good to catch the case and give an informative error
| ## Add singleton dimension | ||
|
|
||
| This release introduces `push_dim` which allows the user to add a singleton | ||
| dimension to end of the dimensions of a `OutputVar`. |
There was a problem hiding this comment.
| dimension to end of the dimensions of a `OutputVar`. | |
| dimension to the end of the dimensions of a `OutputVar`. |
| error( | ||
| "Dimension ($dim_name) cannot be added as it already corresponds to an existing dimension ($conventional_dim_names)", | ||
| ) |
There was a problem hiding this comment.
| error( | |
| "Dimension ($dim_name) cannot be added as it already corresponds to an existing dimension ($conventional_dim_names)", | |
| ) | |
| existing_dim = findfirst(d -> conventional_dim_name(d) == conventional_dim_name(dim_name), keys(dims)) | |
| error( | |
| "Dimension \"$dim_name\" cannot be added as it already corresponds to an existing dimension \"$existing_dim\" (both map to \"$(conventional_dim_name(dim_name))\")" | |
| ) |
Minor, but it would be good to print the conflicting dimension for the user.
| @test size(lat_var.data) == (2, 1) | ||
|
|
||
| # Error handling | ||
| @test_throws ErrorException ClimaAnalysis.push_dim(var, "lon", 42.0) |
There was a problem hiding this comment.
Could you add a test for adding a dimension with a different name but same conventional name? For example, adding a "longitude" dimension since the "lon" dimension exists already:
@test_throws ErrorException ClimaAnalysis.push_dim(var, "longitude", 42.0)
closes #353 - This PR adds
Var.push_dimwhich adds an extra dimension of length 1 to aOutputVar.