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

Feat/cog overviews #90

Merged

Conversation

wildintellect
Copy link
Contributor

New notebook showing an in-depth comparison of overview resampling methods.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

review-notebook-app bot commented Nov 13, 2023

View / edit / reply to this conversation on ReviewNB

kylebarron commented on 2023-11-13T22:41:22Z
----------------------------------------------------------------

Maybe add a hyperlink to titiler?

Upper case GDAL in "available in gdal"?



Copy link

review-notebook-app bot commented Nov 13, 2023

View / edit / reply to this conversation on ReviewNB

kylebarron commented on 2023-11-13T22:41:23Z
----------------------------------------------------------------

You say "The original file does not contain overviews" but it shows IFDs 1, 2, and 3 have positive decimations...? I thought that meant this file does contain some overviews.


wildintellect commented on 2023-11-13T23:31:00Z
----------------------------------------------------------------

Oh, right since I changed the example file to an updated one that's actually a COG. I suppose I can just drop that part of the sentence.

wildintellect commented on 2023-11-15T00:50:14Z
----------------------------------------------------------------

reworded

Copy link

review-notebook-app bot commented Nov 13, 2023

View / edit / reply to this conversation on ReviewNB

kylebarron commented on 2023-11-13T22:41:24Z
----------------------------------------------------------------

Line #6.    OverviewResampling = [r for r in ResamplingEnums if (r.value < 8) or r.value > 13]

I think this is pretty hard for someone to follow because they have to know the specific ordering of items in ResamplingEnum as well as what those mean. Even if it's verbose, I'd suggest having a more literal list like just

resample_methods = [
Resampling.nearest,
Resampling.bilinear,
...

]



wildintellect commented on 2023-11-13T23:35:14Z
----------------------------------------------------------------

It's dynamic though, if/when GDAL adds more options they automatically are part of the list(which did happen during the original implementation of this code), assuming GDAL doesn't change the order. I'll think about how to simplfy, thanks.

kylebarron commented on 2023-11-13T23:42:53Z
----------------------------------------------------------------

I think as long as you don't exclude by number, it'll read more clearly. How about:

from rasterio.enums import Resampling

excluded_resamplings = {
Resampling.max,
Resampling.min,
Resampling.med,
Resampling.q1,
Resampling.q3,
}
resample_methods = [
method for method in Resampling if method not in excluded_resamplings
]
resample_methods

[<Resampling.nearest: 0>,

<Resampling.bilinear: 1>,

<Resampling.cubic: 2>,

<Resampling.cubic_spline: 3>,

<Resampling.lanczos: 4>,

<Resampling.average: 5>,

<Resampling.mode: 6>,

<Resampling.gauss: 7>,

<Resampling.sum: 13>,

<Resampling.rms: 14>]

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Some comments on readability and overviews, but otherwise looks good

Copy link
Contributor Author

Oh, right since I changed the example file to an updated one that's actually a COG. I suppose I can just drop that part of the sentence.


View entire conversation on ReviewNB

Copy link
Contributor Author

It's dynamic though, if/when GDAL adds more options they automatically are part of the list(which did happen during the original implementation of this code), assuming GDAL doesn't change the order. I'll think about how to simplfy, thanks.


View entire conversation on ReviewNB

Copy link
Member

I think as long as you don't exclude by number, it'll read more clearly. How about:

from rasterio.enums import Resampling

excluded_resamplings = {
Resampling.max,
Resampling.min,
Resampling.med,
Resampling.q1,
Resampling.q3,
}
resample_methods = [
method for method in Resampling if method not in excluded_resamplings
]
resample_methods

[<Resampling.nearest: 0>,

<Resampling.bilinear: 1>,

<Resampling.cubic: 2>,

<Resampling.cubic_spline: 3>,

<Resampling.lanczos: 4>,

<Resampling.average: 5>,

<Resampling.mode: 6>,

<Resampling.gauss: 7>,

<Resampling.sum: 13>,

<Resampling.rms: 14>]


View entire conversation on ReviewNB

Copy link
Contributor Author

reworded


View entire conversation on ReviewNB

@wildintellect wildintellect merged commit fed3ab9 into cloudnativegeo:staging Nov 15, 2023
2 checks passed
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.

2 participants