geospatial-jeff / aiocogeo

Asynchronous cogeotiff reader
MIT License
72 stars 10 forks source link

Add ZOOM_LEVEL_STRATEGY for picking overview zoom levels #96

Closed dmahr1 closed 3 years ago

dmahr1 commented 3 years ago

Work done

Links

kylebarron commented 3 years ago

Man that's so much easier in Numpy :smile:

dmahr1 commented 3 years ago

@geospatial-jeff I have very little experience contributing back to open source so please let me know if I'm not following decorum or best practices.

As discussed in the cogeotiff Slack, this is a work in progress PR for picking the closest overview level in a more reliable way. I would love to add some tests, but I am getting a large number of failures when I run pytest tests/ in a dedicated virtual environment. I think it has to do with accessing data via s3:// style links. I'm kind of a noob with boto (I use Google Cloud day-to-day) so perhaps there's some kind of initialization I'm neglecting?

Also, is there any way to run CI on PRs or is it only for builds of master?

geospatial-jeff commented 3 years ago

Also, is there any way to run CI on PRs or is it only for builds of master?

I think now that we've switched to github actions this might run CI on your next commit. But I'm not 100% sure. If it doesn't i'll have to take a deeper look.

I would love to add some tests, but I am getting a large number of failures when I run pytest tests/ in a dedicated virtual environment.

It should be easier now that we are using tox (thanks @vincentsarago ). I think you do need aws credentials configured in your environment or via aws configure (otherwise boto is unhappy), but it doesn't matter what your creds are as its accessing data in a public bucket. With #98 we can remove this dependency.

kylebarron commented 3 years ago

this might run CI on your next commit

I think you need to merge master into your branch

dmahr1 commented 3 years ago

@geospatial-jeff @kylebarron Thanks for the suggestions! I pulled upstream, got my aws CLI configured, and on the master branch all tests are now passing. A few tests are failing on my PR branch but they are related to my change, so I'll fix them up and add some new tests as needed.

The CI did run but it ran into the same S3 credentials issue. It looks like @vincentsarago is looking into that in #100.

Really appreciate your help and patience here. 👍

dmahr1 commented 3 years ago

@geospatial-jeff I've refactored this PR such that the ZOOM_LEVEL_STRATEGY environment variable controls the behavior. The business logic is not quite as elegantly simple as it was with np.argmin but it's a lot more flexible now. I put an explanation in the README (below) but I am open to other suggestions.

  • ZOOM_LEVEL_STRATEGY - mimics GDAL's ZOOM_LEVEL_STRATEGY creation option:
    • AUTO or 50 (default) upsamples or downsamples the zoom level whose resolution is closest to the desired resolution.
    • LOWER or 100 always upsamples the zoom level immediately below the desired resolution (requesting less data).
    • UPPER or 0 always downsamples the zoom level immediately above the desired resoluion (requesting more data).
    • Another integer from 0 through 100: if the desired resolution more this percentage of the way from the zoom level immediately below to the zoom level immediately above, then upsample the zoom level immediately below, else downsample the zoom level immediately above. For example, 1 is the same as UPPER unless the COG's resolution is very close to the zoom level below e.g. due to floating point imprecision.

Also all tests are succeeding for me locally, but they are always failing in CI. Do you or @vincentsarago have any suggestions to fix the CI stuff?

P.S. Happy new year!

vincentsarago commented 3 years ago

@dmahr1 I think the CI is falling because of how actions works (and because we are not yet using moto to mock boto3).

geospatial-jeff commented 3 years ago

Yes @vincentsarago is correct