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/oviews #55

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Feat/oviews #55

wants to merge 10 commits into from

Conversation

wildintellect
Copy link
Collaborator

Merge overview comparison branch to main to make it easier for people browsing the repo to find the example.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 17, 2023

View / edit / reply to this conversation on ReviewNB

abarciauskas-bgse commented on 2023-03-17T18:50:25Z
----------------------------------------------------------------

Nit but I think it should be "Cloud Optimized GeoTIFF (COG)" in the title and then we can just use COG every where else in this file.

Link to titiler: https://devseed.com/titiler/


@abarciauskas-bgse
Copy link
Contributor

This is cool - I'm sure you can share with UWG but perhaps also worth demoing (perhaps a part of more comprehensive documentation demo) a the IMPACT sprint review next week?

@wildintellect
Copy link
Collaborator Author

@abarciauskas-bgse made the changed requested concerning COG text usage.

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 20, 2023

View / edit / reply to this conversation on ReviewNB

emileten commented on 2023-03-20T00:35:43Z
----------------------------------------------------------------

TODO: investigate doing in memory with rio-cogeo

I think you are already doing everything in memory here, maybe this comment is out of date ?


wildintellect commented on 2023-03-24T23:00:49Z
----------------------------------------------------------------

You are right that it is now using rio-cogeo but no it's not all in memory, each overview type is getting written to disk and then read back in for the plotting.

emileten commented on 2023-03-27T01:14:54Z
----------------------------------------------------------------

So it would make use of this ? https://github.com/cogeotiff/rio-cogeo/blob/467a6e8b1fb024c78a4d0989e840de39d9188715/rio_cogeo/cogeo.py#L59

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 20, 2023

View / edit / reply to this conversation on ReviewNB

emileten commented on 2023-03-20T00:35:44Z
----------------------------------------------------------------

Line #14.        config = {"GDAL_NUM_THREADS": "ALL_CPUS", "GDAL_TIFF_OVR_BLOCKSIZE": "512"} 

Maybe we could have a line explaining what the purpose of these three lines is ? GDAL_NUM_THREADS Is self-explanatory, but the other things not quite ?


wildintellect commented on 2023-03-24T23:08:21Z
----------------------------------------------------------------

Blocksize is really hard to explain without a breakout box. https://kokoalberti.com/articles/hosting-and-accessing-cloud-optimized-geotiff-files-on-aws-s3/

"Increase the block size to 512 by 512 pixels. This is not a strict requirement, but doing so will decrease the number of HTTP range requests required to retrieve a certain area"

emileten commented on 2023-03-27T02:25:13Z
----------------------------------------------------------------

The block size is essentially the 'chunk' sizes in a giventif file, correct?

emileten commented on 2023-03-27T02:25:31Z
----------------------------------------------------------------

Interesting article by the way thank you !

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 20, 2023

View / edit / reply to this conversation on ReviewNB

emileten commented on 2023-03-20T00:35:45Z
----------------------------------------------------------------

Line #7.        aws_session = AWSSession(boto3.Session())

Why do we need an aws_session (and therefore import boto3 ) at all since we're reading from and writing to the local filesystem in this example ?


wildintellect commented on 2023-03-24T23:10:13Z
----------------------------------------------------------------

this code also works with a remote hosted COG on s3 (probably originally tested that way). Do you think we should drop that part? in the MAAP scenario it's pretty relevant since users can put in s3 files, even if the examples doesn't show that. Maybe I should move the sample data to a bucket?

emileten commented on 2023-03-27T02:33:09Z
----------------------------------------------------------------

I agree it's relevant, but just find it a bit confusing to have it there but not used without explanation. Maybe I would at just a one line comment

// this is here in case the input data is a remotely hosted COG on s3.

aws_session = AWSSession(boto3.Session())

with rio.Env(aws_session):

@emileten
Copy link
Contributor

I agree this is really cool 😄 added a couple comments @wildintellect.

emileten
emileten previously approved these changes Mar 20, 2023
@wildintellect wildintellect marked this pull request as draft March 24, 2023 17:16
@wildintellect
Copy link
Collaborator Author

We discovered a bug in the test data that was used for this example. The NoData value was incorrectly set lauraduncanson/icesat2_boreal#61
This could impact the outputs significantly (might even be an add-on to the post),

However in testing to repeat this notebook is now broken because the dependencies are now out of alignment.

pkg_resources.VersionConflict: (rasterio 1.2.10 (/opt/conda/envs/gdal34/lib/python3.10/site-packages), Requirement.parse('rasterio>=1.3.3'))

So I need to got back and pin all the libs.

Copy link
Collaborator Author

You are right that it is now using rio-cogeo but no it's not all in memory, each overview type is getting written to disk and then read back in for the plotting.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Blocksize is really hard to explain without a breakout box. https://kokoalberti.com/articles/hosting-and-accessing-cloud-optimized-geotiff-files-on-aws-s3/

"Increase the block size to 512 by 512 pixels. This is not a strict requirement, but doing so will decrease the number of HTTP range requests required to retrieve a certain area"


View entire conversation on ReviewNB

Copy link
Collaborator Author

this code also works with a remote hosted COG on s3 (probably originally tested that way). Do you think we should drop that part? in the MAAP scenario it's pretty relevant since users can put in s3 files, even if the examples doesn't show that. Maybe I should move the sample data to a bucket?


View entire conversation on ReviewNB

Copy link
Contributor

So it would make use of this ? https://github.com/cogeotiff/rio-cogeo/blob/467a6e8b1fb024c78a4d0989e840de39d9188715/rio_cogeo/cogeo.py#L59


View entire conversation on ReviewNB

Copy link
Contributor

The block size is essentially the 'chunk' sizes in a giventif file, correct?


View entire conversation on ReviewNB

Copy link
Contributor

Interesting article by the way thank you !


View entire conversation on ReviewNB

Copy link
Contributor

I agree it's relevant, but just find it a bit confusing to have it there but not used without explanation. Maybe I would at just a one line comment

// this is here in case the input data is a remotely hosted COG on s3.

aws_session = AWSSession(boto3.Session())

with rio.Env(aws_session):


View entire conversation on ReviewNB

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.

3 participants