Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow lazy execution of the reprojection method #1082

Merged
merged 15 commits into from
Nov 5, 2024

Conversation

konstntokas
Copy link
Contributor

@konstntokas konstntokas commented Oct 21, 2024

Closes #1079
Allow lazy execution of the resampling_in_space method. Following changes have been implemented:

  • A keyword argument xy_res has been added to the function transform_grid_mapping() in xcube/core/gridmapping transform, which allows the user to set the resolution of the new gridmapping, which speeds up the method, because the estimation of the spatial resolution takes a lot of time if reprojection is required.
  • If no tile_size is specified for the resampled grid mapping, it defaults to the tile_size of the source grid mapping, improving the user-friendliness of resampling and reprojection.

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%)

@konstntokas
Copy link
Contributor Author

konstntokas commented Oct 24, 2024

The following things are missing, which I'd like to further implement:

  • in xcube/core/gridmapping in new_grid_mapping_from_coords: 1d case should also support user defined xy_res
  • unittest for xy_res
  • enhance unittest, so that all if-branches are covered.

@konstntokas konstntokas requested a review from forman October 24, 2024 11:47
@konstntokas
Copy link
Contributor Author

https://github.com/xcube-dev/xcube/blob/main/xcube/core/resampling/rectify.py#L131C1-L137C10

This function called in rectify_dataset() also does not work lazily.

@konstntokas
Copy link
Contributor Author

https://github.com/xcube-dev/xcube/blob/main/xcube/core/resampling/rectify.py#L131C1-L137C10

This function called in rectify_dataset() also does not work lazily.

This now works in a lazy manner.

@konstntokas konstntokas marked this pull request as ready for review October 25, 2024 14:32
@konstntokas konstntokas requested a review from pont-us October 25, 2024 14:45
Copy link
Member

@pont-us pont-us left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions/suggestions but approved since I don't need to review again.

CHANGES.md Outdated

### Fixes

* The function `resampling_in_space` now operates lazily and support chunk-wise,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The function `resampling_in_space` now operates lazily and support chunk-wise,
* The function `resampling_in_space` now operates lazily and supports chunk-wise,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -196,7 +196,7 @@ def resample_in_space(
)
downscaled_dataset = resample_dataset(
source_ds,
((x_scale, 1, 0), (1, y_scale, 0)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the scale factor now inverted in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came across this through an example and realized that it should be inverted. It is not covered by any test.

The code applies these steps if 0.95 target_gm.x_res > source_gm.x_res, meaning that the source has a finer resolution than the target resolution. If this is the case the source dataset shall be resampled to a coarser grid, which has a similar resolution like the target grid. But since x_scale < 0.95, the grid will be even finer. However, the inverse downsamples the source dataset to the desired resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I forot to change it in the if source_gm.is_regular:, which I will correct for in the next push

Copy link
Member

@pont-us pont-us Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think line 184 (downscaled_gm = source_gm.scale((x_scale, y_scale)) ) also needs this inversion.

}
)
if other._xy_coords is None:
_ = other.xy_coords
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From code inspection and context, I realize that we're forcing initialization of other._xy_coords here, but a brief explanatory comment would be welcome :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@forman forman self-assigned this Nov 4, 2024
Comment on lines -179 to -192

# Source has higher resolution than target.
# Downscale first, then rectify
if source_gm.is_regular:
# If source is regular
downscaled_gm = source_gm.scale((x_scale, y_scale))
downscaled_dataset = resample_dataset(
source_ds,
((x_scale, 1, 0), (1, y_scale, 0)),
size=downscaled_gm.size,
tile_size=source_gm.tile_size,
xy_dim_names=source_gm.xy_dim_names,
var_configs=var_configs,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code does not go into this if case. If the source_gm is regular, an affine transformation will be performed in line 151.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@forman forman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work, thank you!

Just accept my suggestions which are concerning documentation only - then you can merge right away.

Comment on lines -179 to -192

# Source has higher resolution than target.
# Downscale first, then rectify
if source_gm.is_regular:
# If source is regular
downscaled_gm = source_gm.scale((x_scale, y_scale))
downscaled_dataset = resample_dataset(
source_ds,
((x_scale, 1, 0), (1, y_scale, 0)),
size=downscaled_gm.size,
tile_size=source_gm.tile_size,
xy_dim_names=source_gm.xy_dim_names,
var_configs=var_configs,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

xcube/core/gridmapping/base.py Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
konstntokas and others added 3 commits November 5, 2024 14:52
Co-authored-by: Norman Fomferra <norman.fomferra@brockmann-consult.de>
Co-authored-by: Norman Fomferra <norman.fomferra@brockmann-consult.de>
Co-authored-by: Norman Fomferra <norman.fomferra@brockmann-consult.de>
@konstntokas konstntokas merged commit 1d2b453 into main Nov 5, 2024
3 checks passed
@konstntokas konstntokas deleted the konstntokas-xxx-gridmapping_lazy branch November 5, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure lazy resampling
3 participants