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.

Sign up for GitHub

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

Jump to bottom

remove individual options attributes #535

Merged
merged 4 commits into from
Oct 13, 2022
Merged

remove individual options attributes #535

merged 4 commits into from
Oct 13, 2022

Conversation

vincentsarago
Copy link
Member

This PR does:

  • remove individual options attributes (nodata, unscale, ...) which are forwarded to the low-level reader and are now replaced with a unique options={} 
    # before
with COGReader("cog.tif", nodata=1, resampling_method="bilinear") as cog:
    data = cog.preview()
# now
with Reader(COGEO, options={"nodata": 1, "resampling_method": "bilinear"}) as cog:
    data = cog.preview()

This PR is built on top #534

geospatial-jeff
geospatial-jeff approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@geospatial-jeff geospatial-jeff left a comment

Choose a reason for hiding this comment

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

Nice! I never liked having to remember which arguments went were. The one downside of this change is code editors won't know what arguments are available. It should be documented somewhere what the possible arguments are for 'options', if not already.

vincentsarago
vincentsarago commented Oct 13, 2022
View reviewed changes
rio_tiler/io/rasterio.py

# Context Manager to handle rasterio open/close
_ctx_stack = attr.ib(init=False, factory=contextlib.ExitStack)
_minzoom: int = attr.ib(init=False, default=None)
_maxzoom: int = attr.ib(init=False, default=None)

@options.default
def _options_default(self):
return {}
Copy link
Member Author

@vincentsarago vincentsarago Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't get mypy to be happy when putting attr.ib(factory={}) for default

vincentsarago
vincentsarago commented Oct 13, 2022
View reviewed changes
rio_tiler/reader.py Outdated
@@ -23,6 +23,16 @@
from rio_tiler.utils import get_vrt_transform, has_alpha_band, non_alpha_indexes


class Options(TypedDict):
Copy link
Member Author

Choose a reason for hiding this comment

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

name?

geospatial-jeff
geospatial-jeff reviewed Oct 13, 2022
View reviewed changes
rio_tiler/reader.py
class Options(TypedDict):
"""Reader Options."""

nodata: Optional[NoData]
Copy link
Member

Choose a reason for hiding this comment

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

I think we want NotRequired here isntead https://stackoverflow.com/questions/72326834/how-to-have-optional-keys-in-typeddict

Copy link
Member

Choose a reason for hiding this comment

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

Also note:

Note: If you import Required or NotRequired from typing_extensions (on Python <3.11), you should also import TypedDict from typing_extensions.

@kylebarron
Copy link
Member

The one downside of this change is code editors won't know what arguments are available

I didn't have time for a close review but I do tend to be wary of this because options dicts or **kwargs can feel like black holes at times, where you don't know what values are allowed/disallowed. Maybe with the typeddict it's clear enough

@vincentsarago vincentsarago merged commit 3292411 into renameCogeo Oct 13, 2022
@vincentsarago vincentsarago deleted the optionsAttrReader branch October 13, 2022 21:45
vincentsarago added a commit that referenced this pull request Oct 13, 2022
@vincentsarago
rename COGReader to Reader and remove relative import (#534)
dbbfb9b 
* rename COGReader to Reader and remove relative import

* rasterio

* debug

* merge from branch

* add note for backwards compatibility of COGReader

* remove individual options attributes (#535)

* remove individual options attributes

* add typeDict for options

* allow empty options
vincentsarago added a commit that referenced this pull request Oct 19, 2022
@vincentsarago @geospatial-jeff
@rabernat @TomAugspurger
[WIP] Rio tiler v4 (#528)
4baf505 
* update rasterio requirement (#517)

* update rasterio requirement

* remove python 3.7

* update changelog

* reader functions returns ImageData object (#515)

* reader functions returns ImageData object

* update changelog

* remove async base reader (#520)

* remove gcp reader (#521)

Co-authored-by: vincentsarago <[email protected]>

* avoid warning in tests

* generalize buffer option (#519)

* generalize buffer option

* refactor buffer calculation

* better tests

* remove min/max zoom from baseclass attribute (#522)

* remove min/max zoom from baseclass attribute

* fix benchmark

* split zooms methods and add tests

* rename function

* move apply_expression into ImageData class and use `b{ix}` for band names (#523)

* move apply_expression into ImageData class and use `b{ix}` for band names

* fix test

* make sure we have the good number of bands

* Apply expression part2 (#525)

* ImageData.apply_expression return a new ImageData object

* refactor Points method and deprecate asset_expression

* remove unused option

* Update rio_tiler/models.py

* fix regex

* add PointData class (#526)

* add PointData class

* fix test

* remove deprecated

* fix test 2

* better expression parsing for stac

* docstring fixes

* only use WarpedVRT when doing reprojection or nodata overwride (#529)

* update baseclass to remove useless kwargs

* update changelog

* forward dataset statistics to ImageClass (#531)

* forward dataset statistics to ImageClass

* better type and autorescale

* one more test

* rename `COGReader` to `Reader` and remove relative import (#534)

* rename COGReader to Reader and remove relative import

* rasterio

* debug

* merge from branch

* add note for backwards compatibility of COGReader

* remove individual options attributes (#535)

* remove individual options attributes

* add typeDict for options

* allow empty options

* make sure to not change the input array

* add ImageReader for non-geo images (#536)

* add ImageReader for non-geo images

* update changelog

* Xarray reader (#530)

* sketch out Xarray support

* Update rio_tiler/io/xarray.py

Co-authored-by: Ryan Abernathey <[email protected]>

* read tile

* assume crs is in the dataset

* upside down

* dependencies

* cleanup

* add more methods and cleanup

* Update rio_tiler/io/xarray.py

Co-authored-by: Tom Augspurger <[email protected]>

* enumerate from 1

* fix

* add dataset_statistics and notebook

* add XarrayReader in top level import

* add tests

* update notebook

* update changelog

Co-authored-by: Ryan Abernathey <[email protected]>
Co-authored-by: Tom Augspurger <[email protected]>

* add migration docs

* update readme

* update notebooks

* update docs

Co-authored-by: Jeff Albrecht <[email protected]>
Co-authored-by: Ryan Abernathey <[email protected]>
Co-authored-by: Tom Augspurger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geospatial-jeff geospatial-jeff geospatial-jeff approved these changes

@kylebarron kylebarron kylebarron left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vincentsarago @kylebarron @geospatial-jeff