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

Dataset improvements #262

Merged
merged 4 commits into from
Oct 7, 2018
Merged

Dataset improvements #262

merged 4 commits into from
Oct 7, 2018

Conversation

m-mohr
Copy link
Collaborator

@m-mohr m-mohr commented Oct 5, 2018

Based on the last meeting (see recap: https://groups.google.com/forum/#!topic/stac-spec/A-7_e-eo3oU) I made this PR to implement the changes (related issues: # #184):

Doesn't yet implement it to the API (needs to be solved with #257) and the decision on the name (#260) is still due.

Copy link
Contributor

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Overall looks great. Put in a few notes to add some more information.

I did raise one thing worth discussing, which is making the requirement of linking to a dataset not in the Item spec. Like put it in catalog/dataset - to make a complaint catalog/dataset all Items referenced from it should refer to a dataset.

dataset-spec/dataset-spec.md Show resolved Hide resolved

| Field Name | Type | Description |
| ---------- | ------ | ------------------------------------------------------------ |
| name | string | **REQUIRED.** The name of the organization or the individual. |
| type | string | The type of provider. Any of `producer`, `processor` or `host`. |
| url | string | Homepage of the provider. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we make a recommendation here that the homepage should ideally include a way to find a contact about the dataset? To help explain a bit why we dropped 'contact' info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, will add that next week.

@@ -110,7 +108,7 @@ link is required to be absolute.

#### Datasets

Items are *strongly recommended* to provide a link to a dataset definition.
Items are *required* to provide a link to a dataset definition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's a bad idea, but I was thinking we could bring the requirement to link to a dataset in at the Catalog level (or the Dataset level). Like let the item spec stand alone, and just have a strong recommendation. And then for Catalog it could add that any Item referenced from the Catalog should have a Dataset.

As I've been explaining the set of specs I've been thinking about trying to make sure they could be used standalone. And requiring a dataset link in an Item does make it less able to stand on its own.

And I think we should put a bit more color here on why this is so strongly recommended. Explain that it provides the license and provider information, as well as additional information about the set of Items.

Copy link
Collaborator Author

@m-mohr m-mohr Oct 5, 2018

Choose a reason for hiding this comment

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

It doesn't really care where things are written down as required - they are still required then. So if we don't want close coupling of things, we can just say that they are strongly recommended and must avoid using requirements between the specs at all. Sure, we can add that items that are linked from in a dataset are required to link back to its dataset, but that's a different thing for me than what you wrote in your recap: "This also lead to requiring a dataset link in every Item". So let's make it that only items linked from a dataset requre a link back. I'll work on that next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed, please review it again.


| Field Name | Type | Description |
| ---------- | ------ | ------------------------------------------------------------ |
| name | string | **REQUIRED.** The name of the organization or the individual. |
| type | string | The type of provider. Any of `producer`, `processor` or `host`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also probably explain a bit more on what 'producer', 'processor' and 'host' mean. Doesn't need to be in this table, could be just below or above. But just a bit of explanation to help people make a decision about which to use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I'll add explanation about it next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the explanation. Please check whether it needs more explanation or not.

@matthewhanson
Copy link
Collaborator

I find it a bit annoying that Item has an ID, but for Datasets we have name rather than ID.

Can we change it so that both have a unique Identifier that are both called id?

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 5, 2018

@matthewhanson I think I discussed that earlier with @cholmes and the problem is that we are following different specifications here. GeoJSON requires using "id" and WFS3 requires using "name". I don't like it, too, but that's how it is. We have this trouble also in other parts of the spec as "name" is used sometimes synonymously for IDs or titles. Only id and title are pretty much self-explanatory.

@matthewhanson
Copy link
Collaborator

Ok, yeah that's too bad. It might be too late in the game to ask WFS folks to change to ID, which is clearly a better term as it's specific. name is not.

At any rate, for other parts of STAC where we can we should avoid name entirely and just use id and title.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 5, 2018

@matthewhanson Just try it and open an issue? I thought the same for some changes I proposed, but it seems as if they are getting adopted in WFS. We can only win ;-)
The other place that immediately comes to mind in STAC is the asset name, see my (last) comment in #139.

Copy link
Contributor

@hgs-msmith hgs-msmith left a comment

Choose a reason for hiding this comment

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

Approved, with addenda covered in the discussion.
+1 on recommending the change from 'name' to 'id' for WFS.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 6, 2018

I created an issue for changing name to id in WFS: opengeospatial/ogcapi-features#171
@matthewhanson @hgs-msmith @cholmes Feel free to support that issue. ;-)

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 6, 2018

Will be changed to id in WFS3, see the issue. :) I already changed the STAC catalog and dataset specification. As name was often used as title in catalogs, I basically removed name and introduced both title and id into catalogs. Is that fine with anybody?

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 6, 2018

All requested changes should be incroporated, please re-check whether everything is as expected.

@m-mohr m-mohr mentioned this pull request Oct 6, 2018
@cholmes
Copy link
Contributor

cholmes commented Oct 6, 2018

I just commented on the WFS issue - I'm not fully convinced Clements fully understood that we wanted to change the 'name' in the response document, not just in the url template names. I remember the discussion about the template names, which I think @hgs-msmith brought up, but don't remember them discussing the response context.

But I suppose we can just go ahead with the change and advocate on that issue, as there's pretty good arguments for it and WFS 3.0 isn't locked in yet. Let's just keep tracking the issue and sounding in, and I'm ok for us to go ahead with 'id' (it just may be that WFS+STAC implementations might have to put in both name and id until it works out).

Copy link
Contributor

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Looks good. I can't seem to find the discussion about the required dataset, but just wanted to say that your changes look great, and captured what I was thinking. And also wanted to acknowledge that yes, it's a bit less than what we talked about on the call, but on further reflection it seemed a further requirement than I was thinking, particularly since license and provider aren't required STAC fields, so moving them to dataset isn't enough justification to make dataset required.

@cholmes
Copy link
Contributor

cholmes commented Oct 6, 2018

Oh, also note that no iterations of this PR actually fixed #136 - the 'version' in dataset is not the version of the STAC specification they are following, it's the version of the dataset.

We'd need to add in the stac_version parameter in to dataset, or catalog.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 7, 2018

WFS id vs name: I actually thought changing the URL template would imply also changing the response, otherwise it would be even more inconsistent than before. I think it is fine to follow the route @cholmes suggested.

I can't seem to find the discussion about the required dataset

Its in the chapter about the relation types. It's not a big discussion, but more a note that using the item rel type makes it required for the linked item to refer back to its dataset. Do you see it now?

it's a bit less than what we talked about on the call

But you still think it is good as it is now or should we still change/improve something?

Regarding stac_version (#136): I was not sure whether I was excpected to add that or not, but I can make a separate PR for it.

@cholmes
Copy link
Contributor

cholmes commented Oct 7, 2018

I can't seem to find the discussion about the required dataset

Its in the chapter about the relation types. It's not a big discussion, but more a note that using the item rel type makes it required for the linked item to refer back to its dataset. Do you see it now?

Oh I just meant I couldn't find our github discussion to respond in the thread. I found the edits in the spec - they look great. And yes, I think it's good as it is right now.

Regarding stac_version (#136): I was not sure whether I was excpected to add that or not, but I can make a separate PR for it.

No, I wasn't expecting you to add it. But a separate PR on it would be great.

@matthewhanson matthewhanson merged commit 575167d into dev Oct 7, 2018
@matthewhanson matthewhanson deleted the datasets-v2 branch October 7, 2018 19:48
@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 7, 2018

Oh I just meant I couldn't find our github discussion to respond in the thread. I found the edits in the spec - they look great. And yes, I think it's good as it is right now.

@cholmes Were you looking for the discussions in #174 and #136 ?

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.

4 participants