Skip to content

Conversation

@jpolchlo
Copy link
Contributor

@jpolchlo jpolchlo commented May 1, 2023

Related Issue(s):

Description:
There is a missing compliance in the raster extension vis-a-vis the item asset extension. It is permissible by the raster extension spec to tuck raster_bands information into the item_assets block on a collection. Raster appears not to support this. In the interest of creating a relatively uniform strategy to query information in collection assets, we may be interested in elevating the item_assets' AssetDefinition to first class status with respect to the raster extension. This PR proposes a change to make that happen.

This connects to the classification extension work because that extension can use the same mechanism.

Notes:

  • pystac.extensions.item_assets.AssetDefinition is defined nearly identically to pystac.Asset, with the difference that the former disallows the href field, while the latter requires it. If we could find a smart way to define AssetDefinition as a subclass of Asset, we could possibly entirely eliminate the need for this PR.
  • AssetDefinition has no notion of an owner, so it's not currently possible to validate that the collection hosting the asset definition has the raster extension registered with it. We may want to add an owner to this class to improve its expressivity. Again, this would be obviated by proposal in the above comment.

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

@jpolchlo jpolchlo marked this pull request as ready for review May 1, 2023 20:53
@jpolchlo jpolchlo requested a review from gadomski May 1, 2023 20:53
@gadomski gadomski added this to the 1.8 milestone May 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2023

Codecov Report

Patch coverage: 91.17% and project coverage change: -0.01 ⚠️

Comparison is base (f10a7de) 90.33% compared to head (6f22014) 90.33%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1110      +/-   ##
==========================================
- Coverage   90.33%   90.33%   -0.01%     
==========================================
  Files          48       48              
  Lines        6323     6343      +20     
  Branches      943      944       +1     
==========================================
+ Hits         5712     5730      +18     
- Misses        429      431       +2     
  Partials      182      182              
Impacted Files Coverage Δ
pystac/extensions/item_assets.py 95.37% <85.71%> (-0.75%) ⬇️
pystac/extensions/raster.py 90.46% <92.59%> (+0.14%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jpolchlo
Copy link
Contributor Author

jpolchlo commented May 2, 2023

After in-person discussion, I'll be adding an owner field to the asset definition. Regularizing AssetDefinition with Asset didn't seem to pass the sniff test with @gadomski. Womp womp.

@gadomski
Copy link
Member

gadomski commented May 2, 2023

Regularizing AssetDefinition with Asset didn't seem to pass the sniff test with @gadomski. Womp womp.

I'm probably more of a -0 than -1 on this idea ... I could be on board with trying to share behavior between the two, but I worry about over-abstracting what is really a pretty simple data structure.

@jpolchlo
Copy link
Contributor Author

jpolchlo commented May 2, 2023

I worry about over-abstracting what is really a pretty simple data structure.

Yeah, that tracks. It's easy to enrich asset definitions, so it's not a huge problem. Many extension specs are written with item:assets in there, but many are not. Treating AssetDefinition too much like Asset opens the door to too much "accidental" support for item assets in extensions that didn't mean to have it. Plus the href field is pretty fundamental to Asset-ness, and to not have it is inviting bugs. Let's just leave it out.

@jpolchlo jpolchlo force-pushed the feature/raster-collection-extension branch from df4a379 to 56bbec0 Compare May 2, 2023 19:38
@gadomski gadomski self-requested a review May 3, 2023 12:48
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Needs CHANGELOG entry, otherwise LGTM.

@jpolchlo jpolchlo force-pushed the feature/raster-collection-extension branch from 8937781 to d9fd08e Compare May 4, 2023 22:26
@jpolchlo
Copy link
Contributor Author

jpolchlo commented May 4, 2023

This is ready for merge when tests pass.

@jpolchlo jpolchlo requested a review from gadomski May 11, 2023 14:00
@gadomski gadomski force-pushed the feature/raster-collection-extension branch from f861b49 to 8751c94 Compare May 11, 2023 18:44
@gadomski gadomski enabled auto-merge May 11, 2023 18:44
@gadomski gadomski added this pull request to the merge queue May 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch May 11, 2023
@gadomski gadomski force-pushed the feature/raster-collection-extension branch from 8751c94 to 6f22014 Compare May 11, 2023 18:54
@gadomski gadomski enabled auto-merge May 11, 2023 18:55
@gadomski gadomski added this pull request to the merge queue May 11, 2023
Merged via the queue into stac-utils:main with commit 684b317 May 11, 2023
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.

3 participants