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

Xarray Assets Extension class #1161

Merged
merged 13 commits into from
Jun 22, 2023
Merged

Xarray Assets Extension class #1161

merged 13 commits into from
Jun 22, 2023

Conversation

jsignell
Copy link
Member

@jsignell jsignell commented Jun 14, 2023

Related Issue(s):

Description:

EDIT: I removed the open method, so this is just a regular extension class now.

This PR implements an extension class for xarray-assets extension with a special open method for extended pystac.Assets as described in #846 (comment)

import pystac
from pystac.extensions.xarray_assets import XarrayAssetsExtension

daymet_collection = pystac.Collection.from_file(
    "https://planetarycomputer.microsoft.com/api/stac/v1/collections/daymet-daily-hi"
)
daymet_asset = daymet_collection.assets["zarr-abfs"]
xr_ext = XarrayAssetsExtension.ext(daymet_asset, add_if_missing=True)
xr_ext.open()

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.

@jsignell
Copy link
Member Author

Not super ergonomic with the current extension access system, but I'm optimistic that the extension API can be revamped a la #1051 without a substantial rewrite of the classes.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 94.66% and project coverage change: +0.02 🎉

Comparison is base (dd999cd) 91.96% compared to head (6bf65aa) 91.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1161      +/-   ##
==========================================
+ Coverage   91.96%   91.99%   +0.02%     
==========================================
  Files          50       51       +1     
  Lines        6720     6795      +75     
  Branches      994     1000       +6     
==========================================
+ Hits         6180     6251      +71     
- Misses        365      368       +3     
- Partials      175      176       +1     
Impacted Files Coverage Δ
pystac/extensions/xarray_assets.py 94.59% <94.59%> (ø)
pystac/__init__.py 94.64% <100.00%> (+0.09%) ⬆️

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

@jsignell jsignell requested a review from gadomski June 14, 2023 20:29
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.

Couple high level questions:

  • Did you consider adding xpystac as a optional dependency (ala orjson)?
  • How do you see this model working for "containers" that aren't tied to a specific extension? E.g. @TomAugspurger's proposed to_geopandas for an ItemCollection?
  • I think this deserves more than just API docs, e.g. updating how-to-read-data-from-stac.ipynb to use the new extension interface?

pystac/extensions/xarray_assets.py Outdated Show resolved Hide resolved
pystac/extensions/xarray_assets.py Outdated Show resolved Hide resolved
@jsignell
Copy link
Member Author

* Did you consider adding **xpystac** as a optional dependency (ala **orjson**)?

I guess that's a good idea in case we need to pin it later on?

* How do you see this model working for "containers" that aren't tied to a specific extension? E.g. @TomAugspurger's proposed `to_geopandas` for an `ItemCollection`?

Because ItemCollections can't have extensions or because there would be no need for a geopandas extension? I was kind of thinking the table extension could be the geopandas extension. I guess alternatively there could be a generic reader extension but that might be too far of a stretch from what extensions are meant to be.

* I think this deserves more than just API docs, e.g. updating how-to-read-data-from-stac.ipynb to use the new extension interface?

Yeah definitely. I wasn't sure if people would like this idea, so I didn't bother with that yet.

@gadomski
Copy link
Member

I guess that's a good idea in case we need to pin it later on?

Yeah, and it makes it clearer that there's pystac functionality that uses xpystac, instead of just tests.

ItemCollections can't have extensions

Yup, this.

I was kind of thinking the table extension could be the geopandas extension.

But theoretically you could want to make a dataframe out of something that doesn't have the table extension in its stac_extensions list, correct?

Yeah definitely. I wasn't sure if people would like this idea, so I didn't bother with that yet.

👍🏼

@jsignell
Copy link
Member Author

But theoretically you could want to make a dataframe out of something that doesn't have the table extension in its stac_extensions list, correct?

Yeah but couldn't you just add the table extension in that case?

@gadomski
Copy link
Member

Yeah but couldn't you just add the table extension in that case?

If you don't need the table extension to put the STAC stuff into a data frame, IMO you shouldn't have to add the table extension. Otherwise, I could see a scenario where someone loads some STAC, adds the table extension to do some data frame work, then has to strip the table extension back out again before saving the STAC somewhere else -- feels hackey.

@jsignell jsignell requested a review from gadomski June 22, 2023 14:56
@jsignell jsignell self-assigned this Jun 22, 2023
@jsignell jsignell added this pull request to the merge queue Jun 22, 2023
Merged via the queue into stac-utils:main with commit f9e5bd6 Jun 22, 2023
@jsignell jsignell deleted the xarray-ext branch June 22, 2023 16:39
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.

2 participants