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

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

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

vincentsarago
Copy link
Member

this PR does:

  • add apply_expression into ImageData class to avoid code duplication (this is possible because we now have a proper ImageData object out the reader methods.)
  • add b prefix for band names breaking. We should have done this from the beginning!!! (e.g if you pass expression="b1,b2" band names would have been 'b1', 'b2' while indexes=(1, 2) would have given ('1', '2')
    # before
    with COGReader("cog.tif") as cog:
        img = cog.read()
        print(cog.band_names)
        >>> ["1", "2", "3"]
    
        print(cog.info().band_metadata)
        >>> [("1", {}), ("2", {}), ("3", {})]
    
        print(cog.info().band_descriptions)
        >>> [("1", ""), ("2", ""), ("3", "")]
    
        print(list(cog.statistics()))
        >>> ["1", "2", "3"]
    
    # now
    with COGReader("cog.tif") as cog:
        img = cog.read()
        print(img.band_names)
        >>> ["b1", "b2", "b3"]
    
        print(cog.info().band_metadata)
        >>> [("b1", {}), ("b2", {}), ("b3", {})]
    
        print(cog.info().band_descriptions)
        >>> [("b1", ""), ("b2", ""), ("b3", "")]
    
        print(list(cog.statistics()))
        >>> ["b1", "b2", "b3"]
    
    with STACReader("stac.json") as stac:
        print(stac.tile(701, 102, 8, assets=("green", "red")).band_names)
        >>> ["green_b1", "red_b1"]

@vincentsarago vincentsarago added the breaking breaking change label Sep 6, 2022
expression=asset_expression.get(asset), # type: ignore
**kwargs,
)
data = cog.tile(*args, indexes=idx, expression=expr, **kwargs)
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated to the PR goal!

blocks = get_expression_blocks(expression)
output.data = apply_expression(blocks, assets, output.data)
output.band_names = blocks
img.apply_expression(expression, bands=assets) # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

here we overwrite the bands that will be use as expression.

FYI:

  • we now have band_names = ["green_b1", "red_b1"]
  • up to now we are assuming that expression=green/red mean there is only one band in each dataset THIS IS BAD
  • in this line we tell apply_expression to overwrite the bands_names to ["green", "red"] to keep the compatibility.
  • The proper expression should be expression="green_b1/red_b1" but this will be maybe too big of a breaking change

@geospatial-jeff any advice here?

Copy link
Member Author

Choose a reason for hiding this comment

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

opened #524 to talk about this

Copy link
Member

Choose a reason for hiding this comment

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

The proper expression should be expression="green_b1/red_b1" but this will be maybe too big of a breaking change

I think we should do something like this. We are breaking things anyways so might as well get it right now. Representing the full hierarchy of asset::band seems like the best way to support applying expressions across multi-band assets.

@@ -59,86 +59,88 @@ def _get_band_url(self, band: str) -> str:
def test_MultiBandReader():
"""Should work as expected."""
with BandFileReader(PREFIX) as cog:
assert cog.bands == ["b1", "b2"]
assert cog.bands == ["band1", "band2"]
Copy link
Member Author

Choose a reason for hiding this comment

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

changed the file names to make it more explicit

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.

Left some thoughts in #524

* 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
@vincentsarago vincentsarago merged commit 470d4a8 into rio-tiler-v4 Sep 14, 2022
@vincentsarago vincentsarago deleted the ApplyExpression branch September 14, 2022 08:48
vincentsarago added a commit that referenced this pull request Oct 19, 2022
* 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
Labels
4.0.0 breaking breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants