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

Solves #408 (incompatibility between crop and downsampling) #448

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

adehecq
Copy link
Member

@adehecq adehecq commented Jan 24, 2024

Resolves #408.

Phew, this was a tough one! It turns out that Raster.crop() was not very thoroughly implemented in the case when data is not loaded. A few things to pay attention to:

  • one needs to create a Window using the on-disk transform, not the in-memory one, otherwise we're just reading the wrong bits!
  • calling rio.DatasetReader.read is actually more complex than it seems when either of cropping or downsampling need to be executed. One needs to make sure that Window and out_shape really match. The window shape can actually be misleading because with option boundless=False (default), the window will be cropped if it extends beyond the raster extent. It makes sense after all...

Doing so, I noticed that our downsampling functionality is actually slightly wrong, see issue #447. Basically, the sampling interval is not exactly constant.
Because of that, one does not get exactly the same result if cropping a downsampled raster with or without data loaded (see commented test). This should be fixed together with #447.

Decided to also modify Raster.info() to print to screen by default, as mentioned in #361.

Copy link
Member

@rhugonnet rhugonnet left a comment

Choose a reason for hiding this comment

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

Amazing! No comment on the code!

My only question: While you are still in this, what do you think is the best way to fix the downsampling issue consistently? #447

@adehecq
Copy link
Member Author

adehecq commented Jan 24, 2024

My only question: While you are still in this, what do you think is the best way to fix the downsampling issue consistently? #447

I made some suggestions in the issue directly. I just don't want to have a look at this now, because I'm sure it will take a few hours to get everything sorted and thoroughly tested...

@adehecq adehecq merged commit 3b5ddc8 into GlacioHack:main Jan 24, 2024
13 checks passed
@adehecq adehecq deleted the fix_crop branch January 24, 2024 15:32
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.

Raster downsampling and cropping operations seem to be incompatible
2 participants