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

add merged_statistics for multiBaseReader #478

Merged
merged 3 commits into from
Feb 14, 2022
Merged

Conversation

vincentsarago
Copy link
Member

ref: developmentseed/titiler#429

In rio-tiler v3 we removed the expression option in the statistics for better consistency (to return Per Assets statistics). We couldn't have between asset expression here because the model would have been different:

  • per asset stats: Dict[str, Dict[str, BandStatistics]] -> {"asset1": {"1": rio_tiler.models.BandStatistics, ...}}
  • merged assets stats: Dict[str, BandStatistics]] -> {"asset1_1": rio_tiler.models.BandStatistics, ...}

Note: Using expression within the MultiBaseReader is really tricky because I guess most of the use case is to use it when STAC is hosting band per file dataset (only one band per COG) for which we have specifically design the MultiBandReader. The user should be aware that merged assets expression might fail if the assets don't have the same number of bands.

Ideally we could require the expression to be in form of asset1_b1+asset2_b1 but sadly it's not possible because we are using the expression to get names of the assets we need to fetch (here https://github.com/cogeotiff/rio-tiler/blob/master/rio_tiler/io/base.py#L374-L378) so adding a suffix to the asset name will make the code instable IMO

@geospatial-jeff @kylebarron I'm happy to discuss this a bit more

cc @giswqs

asset_indexes=asset_indexes,
asset_expression=asset_expression,
max_size=max_size,
**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.

to get stats for a merged stac data we use self.preview to first get a merged array for multiple assets

# Check that we can use expression and asset_expression
stats = stac.merged_statistics(
expression="green/red",
asset_expression={"green": "b1*2", "red": "b1+100"},
Copy link
Member Author

Choose a reason for hiding this comment

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

we can still apply per asset expression or indexes in addition to the expression

@giswqs
Copy link

giswqs commented Feb 4, 2022

Thank you for adding this feature. Much appreciated.

@vincentsarago vincentsarago merged commit 90f57e7 into master Feb 14, 2022
@vincentsarago vincentsarago deleted the mergedStatistics branch February 14, 2022 20:27
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.

4 participants