Open dschlaep opened 3 years ago
Looks like I missed the case where you might have a structured string listing multiple grid mappings. This should certainly be fixed. With what you've supplied here, I think there is enough to craft a good test for this behavior.
Question, would you be ok if ncmeta only returned the first or should it return all that it can find in the case that this extended format is used? It's been quite a while since I looked at this code -- so it's going to take a bit for me to reorient.
That would be great! My specific use case that lead me to this issue was a grid mapping specifying axis order (of a netCDF with just one grid mapping).
So, I would be ok if ncmeta only returned the first grid mapping of an extended format with multiple grid mappings. That said, it is not clear to me how the rest of ncmeta (and rev-dependencies, e.g., stars) would currently treat a netCDF with multiple grid mappings -- I didn't have time yet to test this out, e.g., based on “Example 5.10. British National Grid”; maybe such netCDFs currently don't work for multiple reasons?
However, one idea in an attempt to future-proof the function ncmeta:::nc_grid_mapping_atts.data.frame()
would be to add a new argument, e.g., function (x, data_variable = NULL, grid_mapping = NULL)
that would allow a user to specify one of the available grid mappings, with a default picking the first, e.g.,
nc_grid_mapping_atts.data.frame <- function(x, data_variable = NULL, grid_mapping = NULL) {
...
grid_mapping_vars <- dplyr::filter(...
if (is.null(grid_mapping)) {
grid_mapping <- find_first_grid_mapping(grid_mapping_vars$value)
}
grid_mapping_atts <- dplyr::filter(x, variable %in% grid_mapping)
if (nrow(grid_mapping_atts) == 0) {
warning(paste("no grid_mapping attribute with name", shQuote(grid_mapping), "found"))
return(tibble::tibble())
}
grid_mapping_atts <- dplyr::left_join(...
...
}
and
find_first_grid_mapping <- function(grid_mapping_str) {
strsplit(grid_mapping_str, split = ":", fixed = TRUE)[[1]][1]
}
with
library("testthat")
expect_equal(find_first_grid_mapping("crs"), "crs")
# CF-1.7: "Example 5.10. British National Grid"
expect_equal(
find_first_grid_mapping("crsOSGB: x y crsWGS84: lat lon"),
"crsOSGB"
)
# CF-1.8: "Example 5.11. Latitude and longitude on the WGS 1984 datum + CRS WKT"
expect_equal(find_first_grid_mapping("crs: latitude, longitude"), "crs")
I would like to ask if you would consider enhancing
ncmeta
so that it would support extended grid mapping attributes as defined by CF-1.7 and later?CF conventions before 1.7 required the grid_mapping attribute of a variable to be an exact match with the variable name that provided the grid mapping: e.g,
This is what package
ncmeta
supports with the current implementation of the functionncmeta:::nc_grid_mapping_atts.data.frame()
:Newer versions of CF, however, expanded the concept of grid mapping starting with CF-1.7 to handle cases with more than one crs:
This is illustrated by “Example 5.10. British National Grid”
Since CF-1.8 the extended format of the grid_mapping attribute may also provide coordinate axis order, i.e.,
This is illustrated by “Example 5.11. Latitude and longitude on the WGS 1984 datum + CRS WKT”
Unfortunately, the package
ncmeta
does currently not recognize the crs of such files. Because of the exact matching used by functionncmeta:::nc_grid_mapping_atts.data.frame()
, it currently cannot locate the corresponding crs of a grid mapping attribute in the extended format. It seems that the function could be enhanced to support the extended format if it would use partial matches or matches against the substring(s) before colon(s) of the grid mapping attribute.I recognize that additional design decisions would need to be taken for situations with multiple mappings, e.g., maybe pick the first and warn about the others?
A small reproducible example:
Function to create netCDFs illustrating simple and extended grid_mapping
Create example netCDFs
Reading the extended grid_mapping produces a (non-fatal) error that results in no crs
Reading the simple grid_mapping works as expected and results in the correct crs
Cleanup
I used
Many thanks!