Add basic ItemCollection implementation #430

duckontheweb · 2021-06-10T17:26:35Z

Related Issue(s): #

compatibility with pystac-client #371

Description:

Adds ItemCollection class to work with GeoJSON Feature Collections containing only STAC Items. This class can be serialized/deserialized from JSON, and also implements Collection to make it more convenient to loop over items, get an item by index, and check if an Item is included. Note that until we implement more specific STAC object equality (as per #380), assert item in item_collection will always fail unless clone_items=False is used when instantiating the ItemCollection.

This PR also updates the I/O functions in pystac.__init__ to work with the new ItemCollection class in response to #371. Since ItemCollections are not STACObjects, this means that the read functions in that file (read_file, read_dict) are now not guaranteed to return a STACObject. This means users will need to either handle both cases in their code, or cast the result of those functions to the appropriate type. I'm interested in feedback on whether this is worth the change, or if we should keep these functions unchanged and make people use ItemCollection.from_file in order to read ItemCollections.

PR Checklist:

Code is formatted (run scripts/format)
Tests pass (run scripts/test)
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.

cc: @scottyhq

UPDATE 2021-06-10:

Implemented __contains__ to make ItemCollection a typed.Collection since we now have the option not to clone Items on instantiation and we are no longer manipulating the Item links.

matthewhanson · 2021-06-10T20:14:02Z

I think I lean toward STACObject.read_file only ever returning a STACObject. Seems wrong that a class factory function would return something other than that class.

I think it's ok to require users having to actively open an ItemCollection with ItemCollection.from_file. As long as they know that's how you open an ItemCollection.

I know it doesn't meet the request from @scottyhq , but I think that's just because before it was ambiguous on if an ItemCollection was a STACObject, because there was the ItemCollection class in stac-api-spec, and it had stac_version, etc.

Now that it's clear an ItemCollection is just a GeoJSON FeatureCollection with features that are STAC Items, it's reasonable that it you would open it differently.

matthewhanson · 2021-06-10T20:17:54Z

Wait, I think I'm wrong here....I don't think a STACObject function should return a non STACObject.

But the pystac.read_file is just a convenience function, and now that I look at the code I think this is exactly the right way to do it. STACObject still only returns a STACObject, but the pystac.read_file function can return any class defined within the PySTAC library

pystac/item_collection.py

matthewhanson · 2021-06-10T20:55:14Z

Finished going through this, looks really good overall, with some comments above.

I can see eventually having some high level convenience functions in ItemCollection (such as calculating summaries over an ItemCollection), but I think this great for an initial implementation.

codecov-commenter · 2021-06-10T23:57:00Z

Codecov Report

Merging #430 (2bb16a1) into main (8946c9b) will increase coverage by 0.11%.
The diff coverage is 94.82%.

@@            Coverage Diff             @@
##             main     #430       /-   ##
==========================================
  Coverage   89.56%   89.67%    0.11%     
==========================================
  Files          39       40        1     
  Lines        5115     5172       57     
==========================================
  Hits         4581     4638       57     
  Misses        534      534

Impacted Files	Coverage Δ
pystac/item_collection.py	`94.64% <94.64%> (ø)`
pystac/__init__.py	`100.00% <100.00%> ( 2.77%)`	⬆️
pystac/serialization/__init__.py	`91.30% <100.00%> ( 4.34%)`	⬆️
pystac/stac_io.py	`72.46% <0.00%> (ø)`
pystac/serialization/migrate.py	`85.00% <0.00%> ( 1.66%)`	⬆️

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 8946c9b...2bb16a1. Read the comment docs.

duckontheweb · 2021-06-11T19:29:07Z

@lossyrob This should be ready for your review, too, if you want to take a look.

lossyrob

lgtm besides the top level read and write methods, which I commented on. I'm -0.5 for the change to those methods, so if there's someone feels strongly it should be that way I'm fine with things getting merged in as is.

lossyrob · 2021-06-13T18:47:18Z

pystac/__init__.py

@@ -71,7  72,7 @@
 )


-def read_file(href: str) -> STACObject:
+def read_file(href: str) -> Union[STACObject, ItemCollection]:


I'm not sure it makes sense to change these top level package read/write methods to include ItemCollection. It breaks user code that may rely on the STACObject type, forcing the type differentiation to happen by the caller. Also the last two parameters of write_file make it feel a bit shoe-horned. On the other hand, I can see people getting confused about why read_file wouldn't work on an ItemCollection if they didn't know it wasn't a core stac object.

I think my preference would be to keep these methods working with core STAC types, and force users to treat ItemCollections as a separate concept, which makes more sense with the spec as it currently stands.

Yeah, I agree with all of the points you make here, and I think my preference would be to not change the top-level functions as well. If that ends up being the decision, I will add more clear documentation to the ItemCollection class indicating that it is not a STACObject and cannot be read using those top-level methods.

@scottyhq I'm curious how much of a priority it is for you to be able to read Item Collections using pystac.read_file vs. ItemCollection.from_file.

Thanks for all the work on this! Yes, I'm coming at this primarily from someone new to STAC, who is unfamiliar with spec details. The key expectation I have is that after searching a STAC API for data, and saving 'results.json' to work with later, there is an straightforward way to open that file and navigate it. For Python it seems like pystac_client takes care of the searching and pystac should care of the I/O and navigation of the results. I don't think people should have to understand the concepts of ItemCollections versus Collections, Core vs Not, for this fundamental workflow.

So if pystac.read_file can't handle ItemCollections, and a separate ItemCollection.from_file() is the way forward (or pystac_client.read_file()`?), I think that just needs to be clearly documented.

Also useful (and I think a non-breaking change) would then be for pystac.read_file to have error handing that can recognize the json is an ItemCollection and suggest the correct method to open it, rather than the current KeyError: 'id' ?

Thanks @scottyhq, that all makes sense to me. I'll change those top-level functions back to their original signatures and make sure we have clear docs on how to work with ItemCollections. I'm pretty sure the issue with a KeyError being raised in pystac.read_file is fixed by #402, but I'll add a test to be sure.

@matthewhanson Looking back at the code example in the original issue it seems like the name of the ItemSearch.items_as_collection method might also be misleading. Maybe we should rename that to items_as_item_collection so that users don't think they are saving a STAC Collection?

Removed ItemCollection handling from top-level functions and added test that pystac.read_file raises a STACTypeError instead of the KeyError in 404bb99

@duckontheweb ItemSearch.items_as_collection was misleading. I've renamed it already, there's now get_pages (get the raw JSON of the pages), get_item_collection (gets pages as item collections), get_items (iterator through all pages and items), and get_all_items (gets all items from all pages and returns as single ItemCollection). This matches the get_ syntax used in PySTAC

duckontheweb · 2021-06-14T01:40:21Z

@lossyrob It also just occurred to me that including ITEMCOLLECTION in the STACObjectTypes enum might be misleading as well. It looks like removing that would impact the migration and validation scripts (since it's part of the schema URI mapping), but I think we could work around that. What do you think about removing that from the Enum?

lossyrob · 2021-06-14T01:56:05Z

@duckontheweb I agree that's not great, and should have been removed when ItemCollection was dropped. I'm 1 for dropping it from that Enum and using another separate constant in the migration/validation places where needed.

duckontheweb · 2021-06-14T15:11:17Z

@duckontheweb I agree that's not great, and should have been removed when ItemCollection was dropped. I'm 1 for dropping it from that Enum and using another separate constant in the migration/validation places where needed.

I removed this in 47017df. I ended up just removing any ItemCollection references in the DefaultSchemaUriMap and in the migration code rather than try to patch it. My thinking is that we're not claiming to support ItemCollections as STACObjects, and since they won't appear while crawling a catalog this shouldn't negatively affect anything outside of users who are working with ItemCollections directly.

duckontheweb · 2021-06-14T15:11:50Z

@lossyrob @matthewhanson Ready for another review whenever you have time. Thanks!

matthewhanson · 2021-06-14T16:42:01Z

@duckontheweb Looks good to me to merge!

stac-utils/pystac#430 implemented `pystac.ItemCollection`, which is being removed from `pystac-client` in the next release. This PR adds similar handling for `pystac.ItemCollection`.

duckontheweb added 5 commits June 10, 2021 13:10

Basic ItemCollection implementation

8bf40b6

Add ItemCollection to top-level I/O functions

5d58dc1

Add ItemCollection documentation

2a8cfa1

Fix lint issues

c6b461b

Update CHANGELOG

61b12d1

duckontheweb requested review from lossyrob and matthewhanson June 10, 2021 17:28

duckontheweb linked an issue Jun 10, 2021 that may be closed by this pull request

compatibility with pystac-client #371

Closed

duckontheweb added this to the 1.0.0-rc.1 milestone Jun 10, 2021

matthewhanson reviewed Jun 10, 2021

View reviewed changes

pystac/item_collection.py Outdated Show resolved Hide resolved

matthewhanson reviewed Jun 10, 2021

View reviewed changes

pystac/item_collection.py Outdated Show resolved Hide resolved

matthewhanson reviewed Jun 10, 2021

View reviewed changes

pystac/item_collection.py Outdated Show resolved Hide resolved

duckontheweb added 3 commits June 10, 2021 19:47

Option to not clone Items when instantiating ItemCollection

9b1fed2

Make ItemCollection a typing.Collection

542f8c6

Allow Iterable items argument to ItemCollection

31beb9d

duckontheweb mentioned this pull request Jun 11, 2021

Casting root Catalog to Catalog type interferes with pystac-client Client class #410

Closed

duckontheweb added 4 commits June 11, 2021 10:52

Do not clone Items in ItemCollection.from_dict

698c54b

Accept Iterable of dicts or Items in ItemCollection.__init__

702a26f

Ability to add ItemCollections

1ead0d5

Default to not cloning Items on instantiation

97354cd

duckontheweb requested a review from matthewhanson June 11, 2021 19:28

matthewhanson approved these changes Jun 13, 2021

View reviewed changes

lossyrob reviewed Jun 13, 2021

View reviewed changes

Remove ItemCollection from top-level functions

2bb16a1

duckontheweb force-pushed the add/371-item-collection branch from 404bb99 to 2bb16a1 Compare June 14, 2021 01:31

Remove ITEMCOLLECTION from STACObjectTypes

47017df

duckontheweb requested review from matthewhanson and lossyrob June 14, 2021 15:11

lossyrob approved these changes Jun 14, 2021

View reviewed changes

duckontheweb merged commit 46fbaa7 into stac-utils:main Jun 14, 2021

duckontheweb deleted the add/371-item-collection branch June 14, 2021 17:03

TomAugspurger mentioned this pull request Jun 20, 2021

Adding ItemCollection to PySTAC #302

Closed

TomAugspurger mentioned this pull request Jul 6, 2021

Support pystac.ItemCollection gjoseph92/stackstac#64

Merged

gadomski mentioned this pull request Nov 8, 2022

cannot unpack non-iterable STACVersionRange object #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic ItemCollection implementation #430

Add basic ItemCollection implementation #430

duckontheweb commented Jun 10, 2021 •

edited

Loading

matthewhanson commented Jun 10, 2021

matthewhanson commented Jun 10, 2021

matthewhanson commented Jun 10, 2021 •

edited

Loading

codecov-commenter commented Jun 10, 2021 •

edited

Loading

duckontheweb commented Jun 11, 2021

lossyrob left a comment

lossyrob Jun 13, 2021

duckontheweb Jun 13, 2021

scottyhq Jun 13, 2021

duckontheweb Jun 14, 2021 •

edited

Loading

duckontheweb Jun 14, 2021

matthewhanson Jun 14, 2021

duckontheweb commented Jun 14, 2021

lossyrob commented Jun 14, 2021

duckontheweb commented Jun 14, 2021

duckontheweb commented Jun 14, 2021

matthewhanson commented Jun 14, 2021

Add basic ItemCollection implementation #430

Add basic ItemCollection implementation #430

Conversation

duckontheweb commented Jun 10, 2021 • edited Loading

matthewhanson commented Jun 10, 2021

matthewhanson commented Jun 10, 2021

matthewhanson commented Jun 10, 2021 • edited Loading

codecov-commenter commented Jun 10, 2021 • edited Loading

Codecov Report

duckontheweb commented Jun 11, 2021

lossyrob left a comment

Choose a reason for hiding this comment

lossyrob Jun 13, 2021

Choose a reason for hiding this comment

duckontheweb Jun 13, 2021

Choose a reason for hiding this comment

scottyhq Jun 13, 2021

Choose a reason for hiding this comment

duckontheweb Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

duckontheweb Jun 14, 2021

Choose a reason for hiding this comment

matthewhanson Jun 14, 2021

Choose a reason for hiding this comment

duckontheweb commented Jun 14, 2021

lossyrob commented Jun 14, 2021

duckontheweb commented Jun 14, 2021

duckontheweb commented Jun 14, 2021

matthewhanson commented Jun 14, 2021

duckontheweb commented Jun 10, 2021 •

edited

Loading

matthewhanson commented Jun 10, 2021 •

edited

Loading

codecov-commenter commented Jun 10, 2021 •

edited

Loading

duckontheweb Jun 14, 2021 •

edited

Loading