Minor adjustments and bug fixing of resampling_in_space

[konstntokas]

This PR is related to the previous PR #1098. Further I enhanced the testing of resample_in_space to cover the case of a reprojection, where a subsetting in rectify_dataset is performed.

Checklist:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/source/*
• Changes documented in CHANGES.md
• GitHub CI passes
• AppVeyor CI passes
• Test coverage remains or increases (target 100%)

[codecov]

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.59%. Comparing base (25fdf9f) to head (2855d91).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
xcube/core/resampling/spatial.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1100      +/-   ##
==========================================
+ Coverage   89.58%   89.59%   +0.01%     
==========================================
  Files         277      277              
  Lines       21836    21852      +16     
==========================================
+ Hits        19561    19579      +18     
+ Misses       2275     2273       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TonioF]

This works, but I have the feeling this is a workaround. As the coordinates are included in source_ds, it should be possible to call

GridMapping.from_dataset(source_ds_subset, prefer_crs=source_gm.crs)

and get the correct result.

[konstntokas]

I changed the setup up the source_ds in resample_in_space. I took Sentinel-3 data as inspiration (see notebook https://github.com/xcube-dev/xcube/blob/main/examples/notebooks/resampling/rectify_dataset.ipynb).

[TonioF]

The new solution would cause problems with data variables that have a gridmapping attribute that points to a crs variable which might not exist anymore after these changes. I still prefer the solution of a variable "transformed_spatial_ref". However, if you prefer your way, please make sure the attributes are not wrong.

[konstntokas]

The new solution would cause problems with data variables that have a gridmapping attribute that points to a crs variable which might not exist anymore after these changes. I still prefer the solution of a variable "transformed_spatial_ref". However, if you prefer your way, please make sure the attributes are not wrong.

Thank you that you are looking into it so carefully!

I need to remove the original coordinates, because if the source ds has x and y, then either x and y or transformed_x and transformed_y will be added to the projected_coords in the function xcube.core.gridmapping.cfconv.get_dataset_grid_mapping_proxis, depending if x or transformed_x (same for y) comes first in the list potential_coord_vars. I therefore prefer to setup a clean dataset in resample_in_space, where only one set of coordinates can be found.

I thus would like to keep my suggestion. Further, I delete all grid_mappings from the data variables in the source_ds. I also added a grid_mapping attribute to the unittest.

[TonioF]

I need to remove the original coordinates, because if the source ds has x and y, then either x and y or transformed_x and transformed_y will be added to the projected_coords in the function xcube.core.gridmapping.cfconv.get_dataset_grid_mapping_proxis, depending if x or transformed_x (same for y) comes first in the list potential_coord_vars. I therefore prefer to setup a clean dataset in resample_in_space, where only one set of coordinates can be found.

Do you really have to? For testing, I kept existing variables crs and spatial_ref and added a new one transformed_spatial_ref. Now, even though the source_gm retrieved from GridMapping.from_dataset() was the UTM one, the tests eventually succeeded.

[konstntokas]

I need to remove the original coordinates, because if the source ds has x and y, then either x and y or transformed_x and transformed_y will be added to the projected_coords in the function xcube.core.gridmapping.cfconv.get_dataset_grid_mapping_proxis, depending if x or transformed_x (same for y) comes first in the list potential_coord_vars. I therefore prefer to setup a clean dataset in resample_in_space, where only one set of coordinates can be found.

Do you really have to? For testing, I kept existing variables crs and spatial_ref and added a new one transformed_spatial_ref. Now, even though the source_gm retrieved from GridMapping.from_dataset() was the UTM one, the tests eventually succeeded.

I changed it to transformed_spatial_ref without deleting the previous crs or spatail_ref variable/coordinate. The the CI fails in the newly added test, if actual subsetting is performed, see second test in test_spatial.test_reproject_utm().

[TonioF]

You're right, when I executed this, I had left in the line where x and y were removed from the dataset. I now would accept your solution from before the last two commits. It's still not perfect, but I wouldn't be able to come up with a better solution.

[konstntokas]

@forman can you have a look onto our discussion and my suggested changes. I think a second opinion is useful here. Thank you!

[forman]

This may not be right in cases. You should just drop the grid mapping variable, no matter how it is named. crs and spatial_ref are common names, but their names are not a sufficient condition to identify the grid mapping variable. There can even be more than one such variables if multiple CRSes are used.

[konstntokas]

I now go through the grid_mapping in the data variable attributes, and not only delete the attribute with key key grid_mapping, but also drop the grid mapping variable. Afterwards I still check for the grid mapping variable named with crs and spatial_ref. For Sentinel 2 jp2 files e.g. the dataset has the coordinate spatial_ref, but the data variable has no attribute grid_mapping.

[forman]

Thats good.

[forman]

I like these spacers so the comment is not glued to the next line, but still connected.

[konstntokas]

No preference here. I changed it back. :)

[forman]

@TonioF @konstntokas

I approve unless I overlooked the place where you added transformed_spatial_ref. I'd be against it. Instead replace the existing source grid mapping from. If you need a history, place a record into the global history attribute.