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

Use asset keys as canonical band names #77

Merged
merged 1 commit into from Jun 27, 2022
Merged

Use asset keys as canonical band names #77

merged 1 commit into from Jun 27, 2022

Conversation

gadomski
Copy link
Contributor

Overview

This PR fixes how odc-stac uses the Electro-Optical STAC extension to generate band aliases. Specifically:

  • Asset keys are used as the canonical name for bands, instead of eo:bands -> name
  • eo:bands -> common_name and eo:bands -> name are both used as aliases back to asset keys

This enables support for the common STAC pattern of "one band per asset".

Background

Initially raised in Element84/geo-notebooks#8. USGS has changed the structure of their STAC items such that eo:band -> name does not match the asset key (example item here). Instead, eo:band -> common_name is the asset key.

More generally, odc-stac was incorrectly reading eo:bands information from the Item instead of from the Assets. From the EO extension:

Each Asset should specify its own band object. If the individual bands are repeated in different assets they should all use the same values and include the optional name field to enable clients to easily combine and summarize the bands.

The eo:bands array may optionally be used in the Item Properties to summarize the available bands in the assets. This should be a 'union' of all the possible bands represented in assets. It should be considered merely informative - clients should rely on the eo:bands of each asset.

This PR fixes odc-stac to read band information from each asset.

Details

  • If name or common_name matches the asset key, it is not added to the aliases.
  • Multi-band assets are not supported via the alias mechanism. It's not clear if this was ever correctly supported previously, but now it is explicitly unsupported with a warning.
  • Test case included with a new Landsat item demonstrating its layout.
  • Since this does not change the public API, IMO this could be incorporated in a MINOR release, e.g. v0.3.1.

@github-actions
Copy link

@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #77 (e4ef43c) into develop (064c5f0) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop      #77      +/-   ##
===========================================
+ Coverage    88.37%   88.55%   +0.17%     
===========================================
  Files           19       19              
  Lines         1652     1660       +8     
===========================================
+ Hits          1460     1470      +10     
+ Misses         192      190       -2     
Impacted Files Coverage Δ
odc/stac/_mdtools.py 99.18% <100.00%> (+0.01%) ⬆️
odc/stac/eo3/_eo3converter.py 96.61% <0.00%> (+1.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 064c5f0...e4ef43c. Read the comment docs.

return {}
if eo.bands is None:
continue
if len(eo.bands) != 1:
Copy link
Member

Choose a reason for hiding this comment

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

I'm starting to think that we should probably record all name|common_name -> asset_name, idx and only warn on use when ambiguously named band is requested for loading. Extension seems to encourage using the same common name in particular, so these warnings will get annoying really quick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, asset_key+band_index seems like the right way to make a "band pointer."

@Kirill888 Kirill888 self-requested a review June 27, 2022 21:56
for key, asset in item.assets.items():
try:
eo = EOExtension.ext(asset)
except pystac.errors.ExtensionNotImplemented:
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, this exception will only happen is entire item has not EO so early exit is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, since there is no such thing as asset-level extensions.

Copy link
Member

@Kirill888 Kirill888 left a comment

Choose a reason for hiding this comment

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

Looks like the only thing guaranteed to be a unique identifier for a band is asset name, so let's use that and treat eo:band:name as an alias. Let's merge and release this fix, and think about multi-band support later.

@gadomski
Copy link
Contributor Author

asset name

Just making sure, you mean "key in the assets dictionary", correct?

@Kirill888
Copy link
Member

Kirill888 commented Jun 27, 2022

Just making sure, you mean "key in the assets dictionary", correct?

yes

@Kirill888
Copy link
Member

related: #74

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.

None yet

2 participants