use gridmapping in gen and gen2 #495
Conversation
…use-gridmapping # Conflicts: # xcube/core/gen2/opener.py
AliceBalfanz
left a comment
AliceBalfanz
left a comment
There was a problem hiding this comment.
I have a question at one code place, and also am missing an entry at least in the changelog (I am almost afraid to ask for an entry in the documentation :))
| geo_coding = None | ||
| if crs is not None: | ||
| if spatial_res is None: | ||
| raise ValueError('If CRS is provided, ' |
There was a problem hiding this comment.
Why must the spatial resolution be provided when crs is given? What if a user includes the native CRS of the dataset and only uses the subsetting without resampling?
Codecov Report
@@ Coverage Diff @@
## master #495 +/- ##
==========================================
- Coverage 91.54% 91.50% -0.04%
==========================================
Files 292 292
Lines 26464 26401 -63
==========================================
- Hits 24227 24159 -68
- Misses 2237 2242 +5
Continue to review full report at Codecov.
|
|
|
||
| dataset = select_spatial_subset(dataset, | ||
| xy_bbox=bbox, | ||
| geo_coding=geo_coding) |
| :param dataset: The dataset. | ||
| :param var_names: Optional variable names. | ||
| :param crs: The dataset's CRS. If not set the dataset's current CRS will |
There was a problem hiding this comment.
What is a dataset's current CRS?
| dataset = select_variables_subset(dataset, var_names=var_names) | ||
| if bbox is not None: | ||
| dataset = select_spatial_subset(dataset, xy_bbox=bbox) | ||
| geo_coding = None |
There was a problem hiding this comment.
What does all this code here and why is it not better located in select_spatial_subset.
| y_res = spatial_res[1] if isinstance(spatial_res, tuple) \ | ||
| else spatial_res | ||
| height = math.ceil((y_max - y_min) / y_res) | ||
| geo_coding = GridMapping.regular(size=(width, height), |
There was a problem hiding this comment.
To me, this looks wrong. You cannot assign an arbitrary CRS to a dataset that uses a different CRS. The GridMapping that must first be transformed from one CRS to the datasets CRS.
| from xcube.util.plugin import get_extension_registry | ||
|
|
||
|
|
||
| class ReprojectionInfo: |
There was a problem hiding this comment.
Makes cube incompatible with iproc plugins for "xcube gen"
| from .helpers import _to_affine | ||
| from .helpers import scale_xy_res_and_size | ||
|
|
||
| DEFAULT_CRS = 'EPSG:4326' |
There was a problem hiding this comment.
Default of/for what? I don't think, such default exists. Or at least, we want to get rid of it.
Uses the newly introduced GridMapping in xcube gen and gen2. In particular, there is now a parameter 'crs' that can be passed to the gen cli. The class ReprojectionInfo has been removed.
Checklist:
docs/source/*CHANGES.mdRemember to close associated issues after merge!