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

#94: add EO extension JSON schema #240

Merged
merged 6 commits into from
Oct 7, 2018
Merged

#94: add EO extension JSON schema #240

merged 6 commits into from
Oct 7, 2018

Conversation

jeffnaus
Copy link
Contributor

@jeffnaus jeffnaus commented Sep 5, 2018

Wrote JSON schema based off of https://github.com/radiantearth/stac-spec/blob/dev/extensions/stac-eo-spec.md. I'm doing the same "inheritance" thing where an EO item is still a STAC item which is still a geoJSON feature.

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Great to have a Schema for EO, thanks for the work on it.

I'd suggest the changes mentioned to restrict the values for several of the numeric values.

{
"oneOf": [
{
"$ref": "https://raw.githubusercontent.com/radiantearth/stac-spec/master/json-spec/json-schema/stac-item.json#/definitions/core"
Copy link
Collaborator

@m-mohr m-mohr Sep 5, 2018

Choose a reason for hiding this comment

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

The validator (ajv) we are using in this repository doesn't allow referencing schemas by URL, so we couldn't use this in the CI. Can this be changed to a relative URL? Otherwise I'd just remove the inheritance. Don't see a major advantage here. You could just validate against Item and EO extension separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use relative references then I think we will need all the JSON schemas to live in the same path, i.e. we able to directly construct the path between the relative references. We can do this but we'd have to change the layout to put all JSON schema in one path (like all in one folder), but then you'd either need to put your custom extension schemas there or everybody who uses your custom schema would need to build the appropriate reference resolver. I really put in the absolute reference to get out of the need for custom reference resolvers.

The advantage I see with this inheritance is that we don't need to know the relationship between the various schemas in order to perform the validation. The idea here is that the schema itself says that this extension is extending these various schemas and adding in the specified components.

For instance you could write a schema that requires the item to be both a EO item and a item with the start/stop time extension, plus some other custom fields... The schema would include these various definitions from the extension's schemas and enforce this without the end validation user needing to know to check these other schemas.

Copy link
Collaborator

@m-mohr m-mohr Sep 6, 2018

Choose a reason for hiding this comment

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

I understand all your points made here, but it just won't work with the validator we are using (see ajv-validator/ajv-cli#29 ) and therefore we can't use it in CI. We either need to use another validator or remove the dependency to items for now, I think. (But that's nothing that prevents an approval of the PR.)

"eo:sun_elevation": {
"title": "Sun Elevation",
"description": "Sun elevation angle. The angle from the tangent of the scene center point to the sun. Measured from the horizon in degrees (0-90).",
"type": "number"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"minimum": 0
"maximum": 90

should be added.

"eo:sun_azimuth": {
"title": "Sun Azimuth",
"description": "Sun azimuth angle. From the scene center point on the ground, this is the angle between truth north and the sun. Measured clockwise in degrees (0-360).",
"type": "number"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"minimum": 0
"maximum": 360

should be added.

"eo:azimuth": {
"title": "Azimuth",
"description": "Viewing azimuth angle. The angle measured from the sub-satellite point (point on the ground below the platform) between the scene center and true north. Measured clockwise from north in degrees (0-360).",
"type": "number"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"minimum": 0
"maximum": 360

should be added.

"eo:off_nadir": {
"title": "Off Nadir",
"description": "Viewing angle. The angle from the sensor between nadir (straight down) and the scene center. Measured in degrees (0-90).",
"type": "number"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"minimum": 0
"maximum": 90

should be added.

"eo:cloud_cover": {
"title": "Cloud Cover",
"description": "Estimate of cloud cover as a percentage (0-100) of the entire scene. If not available the field should not be provided.",
"type": "integer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

"minimum": 0
"maximum": 100

should be added.

@m-mohr
Copy link
Collaborator

m-mohr commented Sep 5, 2018

By the way, I ran the validatior by calling npm run validate_extension -- -s "../extensions/json-schema/stac-eo-extension.json" -d "../extensions/examples/landsat8-sample.json"
Unfortunately, the landsat samples are not valid (missing eo:platform, eo:constallation and eo:instrument). We should add these fields to the examples, make sure they all validate and also add the above command to the validation readme.

@jeffnaus
Copy link
Contributor Author

jeffnaus commented Sep 6, 2018

@m-mohr I've added the min/max values you suggested. Thanks again for the great proofing you are doing!

@jeffnaus
Copy link
Contributor Author

jeffnaus commented Sep 6, 2018

In testing I noticed that I had the eo:cloud_cover and other fields inside of the bands object and not inside of the properties objects. I've fixed this in the latest commit.

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Thanks! Approving this to get it in, but with the warning that this will never run with the current validation tools we are using due to the external schema. I think we should replace ajv with something better.

@m-mohr
Copy link
Collaborator

m-mohr commented Sep 7, 2018

By the way, the related issue is #94.

@m-mohr m-mohr changed the title add EO extension JSON schema #94: add EO extension JSON schema Sep 7, 2018
@m-mohr
Copy link
Collaborator

m-mohr commented Sep 21, 2018

Needs to be adopted to the changes introduced by #247 (bands as arrays).

@m-mohr m-mohr changed the base branch from dev to eo-extension-json-schema October 7, 2018 22:11
@m-mohr
Copy link
Collaborator

m-mohr commented Oct 7, 2018

I'll merge this to a development branch in this repository so that we can update the JSON schema by @jeffnaus to the most recent developments.

@m-mohr m-mohr merged commit fcdf41b into radiantearth:eo-extension-json-schema Oct 7, 2018
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.

None yet

2 participants