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

feat: Tweak geospatial type implementation #172

Merged
merged 10 commits into from
Nov 21, 2021

Conversation

l0b0
Copy link
Contributor

@l0b0 l0b0 commented Nov 18, 2021

Closes #170

@l0b0 l0b0 requested review from blacha, dwsilk and billgeo November 18, 2021 00:27
blacha
blacha previously approved these changes Nov 18, 2021
@l0b0 l0b0 force-pushed the feat/tweak-geospatial-type-implementation branch from 8cda80b to 8301f9a Compare November 18, 2021 00:31
extensions/linz/README.md Outdated Show resolved Hide resolved
extensions/linz/README.md Outdated Show resolved Hide resolved
extensions/linz/README.md Outdated Show resolved Hide resolved
extensions/linz/README.md Outdated Show resolved Hide resolved
extensions/linz/README.md Outdated Show resolved Hide resolved
extensions/linz/README.md Outdated Show resolved Hide resolved
extensions/linz/README.md Outdated Show resolved Hide resolved
@l0b0 l0b0 marked this pull request as ready for review November 18, 2021 00:46
@l0b0 l0b0 force-pushed the feat/tweak-geospatial-type-implementation branch from 0b88154 to 54e6cb9 Compare November 18, 2021 00:53
@l0b0 l0b0 requested review from billgeo and blacha November 18, 2021 00:53
extensions/linz/README.md Show resolved Hide resolved
| processing:software | Map<string, string> | Recommended. The software and versions which were used to generate the dataset. See [reference](https://github.com/stac-extensions/processing). |
| Field Name | Type | Description |
| ------------------- | ------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- |
| processing:software | Map<string, string> | Recommended. The software and versions which were used to generate the dataset. See [reference](https://github.com/stac-extensions/processing). |
Copy link
Member

Choose a reason for hiding this comment

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

| Field Name | Type | Description |
| ---------------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| title | string | **REQUIRED**. Collection title. |
| linz:geospatial_type | \[string] | **REQUIRED**. A general description of the type of content that can be found in the dataset. See the [list of accepted geospatial types](#geospatial-types). |
Copy link
Member

Choose a reason for hiding this comment

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

I can't think of a dataset / collection that we hold that would need to be categorised by multiple geospatial types, I guess because we tend to divide datasets / collections based on their "geospatial type". If a summary of all of the geospatial types of the items within the collection is required, then that would seem better implemented as item properties + summaries, but I see this more as a general categorisation which just has one geospatial type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, and it sounds like the documentation cleanup is working if it highlights deficiencies 😁. Do you want to create a separate issue for that though? It seems like it's out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I raised the issue that linz:geospatial_type should be a Collection Field not an Item Field / Item Property for my understanding of its purpose and use. @billgeo suggested leaving it optionally as an Item Property which is fine but not the way I would be implementing it (we could again take the approach of waiting until we find a dataset where it doesn't work as a Collection Field - again - I can't think of an example).

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussion, to keep it simple, let's just leave it as only a collection field for now (remove from items). And change it to be a single string, rather than an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we ever need this for items/assets, we can get into that later when we need it.

Copy link
Contributor Author

@l0b0 l0b0 Nov 18, 2021

Choose a reason for hiding this comment

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

Had a chat with @billgeo and we went with this plan:

  • Remove linz:geospatial_type from item fields, because we don't really need it yet and we don't know what the semantics of having values here which are different from the catalogue value would be.
  • Make linz:geospatial_type a mandatory string (rather than non-empty array of string) type in the collection.

@l0b0 l0b0 requested review from billgeo and dwsilk November 18, 2021 02:56
@l0b0 l0b0 force-pushed the feat/tweak-geospatial-type-implementation branch from 62cedc7 to 437a8f5 Compare November 18, 2021 03:07
@l0b0 l0b0 force-pushed the feat/tweak-geospatial-type-implementation branch from 437a8f5 to 6e60436 Compare November 18, 2021 22:23
Copy link
Contributor

@billgeo billgeo left a comment

Choose a reason for hiding this comment

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

Sorry you had to go around in circles a couple of times. But this LGTM now.

@l0b0
Copy link
Contributor Author

l0b0 commented Nov 19, 2021

Sorry you had to go around in circles a couple of times. But this LGTM now.

No worries, I'd rather get it right than leave it broken :D

@dwsilk dwsilk added the automerge kodiak automerge label label Nov 21, 2021
@kodiakhq kodiakhq bot merged commit 966292d into master Nov 21, 2021
@kodiakhq kodiakhq bot deleted the feat/tweak-geospatial-type-implementation branch November 21, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge kodiak automerge label
Development

Successfully merging this pull request may close these issues.

Feature request: linz:geospatial_type improvements
4 participants