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

eo:bands array #247

Merged
7 commits merged into from
Sep 21, 2018
Merged

eo:bands array #247

7 commits merged into from
Sep 21, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 14, 2018

After discussion, we are now proposing that eo:bands be represented as an array.

This will be particularly helpful when searching for items with band definitions that don't have well known names.

I also updated the extension example to use the new catalog.json inherited metadata model instead of the collections extension.

trying to get all of the examples as well.
@ghost ghost requested review from cholmes, matthewhanson, m-mohr and hgs-msmith September 14, 2018 21:06
@ghost
Copy link
Author

ghost commented Sep 14, 2018

I realized the eo extension didn't discuss the band indexes in the assets. So I added a section describing that as well.

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 for making this PR. Overall I'm fine with these changes, but I was fine with bands as objects, too. To me personally, there not so much difference in both approaches for bands.

I suggest some changes regarding the chapter "Associating assets with bands" and the type definition of eo:bands.

I don't like that we are mixing in some other changes here regarding collections/metadata inheritance. That should be a separate PR that others can discuss separately.

"rel": "self"
}
],
"properties": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't change to the new catalog.json inherited metadata model in this PR as this is not the topic of this PR. That should be separated.

@@ -1,8 +1,8 @@
## Landsat Example with EO and Collection extension
## Landsat Example with EO and common metadata extension
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it now a "common metadata extension"? Did I miss something yesterday?

@@ -21,7 +21,7 @@ It is not necessary, but recommended to use the [Collections extension](stac-col
| eo:platform | string | **REQUIRED.** Unique name of the specific platform the instrument is attached to. For satellites this would be the name of the satellite (e.g., landsat-8, sentinel-2A), whereas for drones this would be a unique name for the drone. |
| eo:constellation | string | **REQUIRED.** Name of the group or constellation that the platform belongs to. Example: The Sentinel-2 group has S2A and S2B, this field allows users to search for Sentinel-2 data without needing to specify which specific platform the data came from. |
| eo:instrument | string | **REQUIRED.** Name of instrument or sensor used (e.g., MODIS, ASTER, OLI, Canon F-1). |
| eo:bands | Map<string, Band Object> | **REQUIRED.** This is a dictionary of band information where each key in the dictionary is an identifier for the band (e.g., "B01", "B02", "B1", "B5", "QA"). |
| eo:bands | List<Band Object> | **REQUIRED.** This is a list of the available bands where each item is a Band Object. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually list arrays as [Band Object] in the other spec files.

@@ -82,13 +84,104 @@ numbers of several popular instruments.
| lwir11 | 10.5 - 11.5 | | | 10 | | 31 |
| lwir12 | 11.5 - 12.5 | | | 11 | | 32 |


#### Associating assets with bands
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added that in #245 as well. I'd suggest mixing both approaches, I have the format description, you have the more human-readable description.

So, I'd suggest changing this a little to:

## Associating assets with bands

Asset definitions that contain band data should reference the band index. Each asset should provide a "eo:bands" property that is an array of 0 based indexes to the correct Band Objects.

### Item `Asset Object` fields
| Field Name | Type     | Description                                  |
| ---------- | -------- | -------------------------------------------- |
| eo:bands   | [number] | Lists the band names available in the asset. |

See [landsat8-merged.json](examples/landsat8-merged.json) for a full example.
...

What I also didn't thought of yesterday, that we wanted to add additional values to this field (for example offset and scale, see #245). So this would change the structure of this field a bit, but shouldn't be a big problem.

@ghost
Copy link
Author

ghost commented Sep 18, 2018

@m-mohr I got a little carried away trying to see what the new catalog format would look like. I agree that it should be done separately, so those parts have been reverted.

@m-mohr
Copy link
Collaborator

m-mohr commented Sep 18, 2018

Thank you for fixing the issues. I think there's one left (see comment). Other than that it's ready for approval.

@ghost
Copy link
Author

ghost commented Sep 18, 2018

@m-mohr I think I addressed them all. Was there a specific comment you are referring too?

@m-mohr
Copy link
Collaborator

m-mohr commented Sep 19, 2018

@hgs-truthe01 Yes, a new one added after your commits. There is still a band objects in the properties object, which imho is not allowed. See the Planet example...

@ghost
Copy link
Author

ghost commented Sep 20, 2018

@m-mohr I didn't notice that I had done that in the example snippets. It should be corrected now.

As a side note, I don't think we should be putting any extensions outside of the properties, but this is not the right place to discuss that!

"type": "Feature",
"properties": {
...
"eo:bands":[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't that be outside of the properties?

@m-mohr
Copy link
Collaborator

m-mohr commented Sep 21, 2018

@m-mohr I didn't notice that I had done that in the example snippets. It should be corrected now.

Thanks. => Approved.

As a side note, I don't think we should be putting any extensions outside of the properties, but this is not the right place to discuss that!

I fully agree with that, but there were these concerns that objects/arrays could not be viewed correctly in legacy(?) GIS systems?!

This pull request was closed.
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.

3 participants