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

initial pass at using pystac instead of sat-stac #72

Merged
merged 14 commits into from
Mar 3, 2021

Conversation

scottyhq
Copy link
Collaborator

this is a work in progress for switching the sat-stac dependency to pystac (https://github.com/stac-utils/pystac)

more discussion here #43

Base automatically changed from master to main February 17, 2021 22:41
@scottyhq scottyhq closed this Feb 17, 2021
@scottyhq scottyhq reopened this Feb 17, 2021
@scottyhq scottyhq closed this Feb 18, 2021
@scottyhq scottyhq reopened this Feb 18, 2021
@scottyhq
Copy link
Collaborator Author

scottyhq commented Feb 18, 2021

not sure why the binder badge workflow is failing, should add a badge like:

body: `[![Binder](https://mybinder.org/badge_logo.svg)](https://mybinder.org/v2/gh/${context.repo.owner}/${context.repo.repo}/${BRANCH_NAME}) :point_left: Launch a binder notebook on this branch`

[![Binder](https://mybinder.org/badge_logo.svg)](https://mybinder.org/v2/gh/scottyhq/intake-stac/pystac) :point_left: Launch a binder notebook on this branch

Binder 👈 Launch a binder notebook on this branch

intake_stac/catalog.py Outdated Show resolved Hide resolved
Comment on lines -361 to -362
f'Stacking failed: {href} does not contain '
'band info in a fixed section of the url'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

previous implementation assumes asset URLs are compatible with the path_as_pattern extraction of coordinate values from the URL. I don't think we should require that. The downside is that you end up with repeating coordinate values by default (e.g. band = [1,1,1] since xr.open_rasterio() defaults to opening each file with band = [1])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any way to intuit the band numbering or ordering. If so, we can always fill in the coordinate values after the fact.

Copy link
Collaborator Author

@scottyhq scottyhq Mar 2, 2021

Choose a reason for hiding this comment

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

@jhamman this is where I get confused. Ultimately we create an intake entry and then delegate opening to intake-xarray plugin (https://github.com/intake/intake-xarray/blob/master/intake_xarray/raster.py). if path_as_pattern is not specified, we return an xarray dataset with bands = [1,1,1...] etc. Consequently, here is what the user interface currently looks like:

list_of_bands = ['B1', 'B2']
stack = item.stack_bands(list_of_bands)
da = stack().to_dask()
da['band'] = list_of_bands 
da

question for @martindurant how to specify the coordinates of the returned xarray dataarray? Unless we can override driver methods in this library, if I understand correctly we'd need to add a kwarg here https://github.com/intake/intake-xarray/blob/1883e30a0faab6a07e7e522bf45bc96e80f62f2c/intake_xarray/raster.py#L60

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that's right.
intake-xarray has emerged as a sort of glue with various options, so it would be inkeeping with current evolution; but we might want to have a chat more generally about what intake-xarray is really trying to achieve. I mean, one might delegate all kwargs to xarray, or one might try to build out a set of helper methods and extra logic that only make sense at this slightly higher level. We currently do a bit of both.

@scottyhq
Copy link
Collaborator Author

@jhamman i'd appreciate some input on this. I view it as important to finish up before tackling other open issues. I'm currently running through notebook examples to get them to work. Would be nice to have them run and be rendered straight into the docs as is done https://corteva.github.io/rioxarray/stable/examples/examples.html . But that be a separate PR.

I also think it would be good to add a number of additional tests. Perhaps mirroring some example files in a tests data folder so that we're not relying on the existence and naming schemes of other repositories. Also seems best for a separate PR. this one is already lots of changes!

Copy link
Collaborator

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It's looking great. A few comments for discussion and pinged a few outside folks that may help make some of the finer design decisions.

intake_stac/catalog.py Outdated Show resolved Hide resolved
intake_stac/catalog.py Show resolved Hide resolved
intake_stac/catalog.py Outdated Show resolved Hide resolved
intake_stac/catalog.py Outdated Show resolved Hide resolved
Comment on lines -361 to -362
f'Stacking failed: {href} does not contain '
'band info in a fixed section of the url'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any way to intuit the band numbering or ordering. If so, we can always fill in the coordinate values after the fact.

intake_stac/catalog.py Outdated Show resolved Hide resolved
intake_stac/catalog.py Outdated Show resolved Hide resolved
intake_stac/catalog.py Show resolved Hide resolved
@TomAugspurger
Copy link
Collaborator

@scottyhq I made a PR to your branch scottyhq#5

Tom Augspurger added 2 commits March 2, 2021 09:06
Collection subclasses catalog, so check it first.
@scottyhq scottyhq marked this pull request as ready for review March 3, 2021 00:46
@scottyhq scottyhq requested a review from jhamman March 3, 2021 00:46
@scottyhq
Copy link
Collaborator Author

scottyhq commented Mar 3, 2021

thanks everyone for the feedback. i'm planning to go ahead and merge as-is. followups:

@TomAugspurger
Copy link
Collaborator

figure out ways to integrate with https://github.com/stac-utils/stac-vrt

I'm happy to take care of that one :)

@scottyhq scottyhq merged commit 191d86f into intake:main Mar 3, 2021
@scottyhq scottyhq deleted the pystac branch March 3, 2021 17:20
@scottyhq scottyhq mentioned this pull request Jun 21, 2021
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.

5 participants