separation of eo, os and sat

[davidraleigh]

Related Issue(s): #722

Proposed Changes:

1. eo contains sun_azimuth and sun_elevation
2. new extension os, Overhead Sensor for containing azimuth, off_nadir, and incidence
3. removal of fields from sat
4. dropping redundant _angle suffix

PR Checklist:

• This PR has no breaking changes.
• I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.
• API only: I have run npm run generate-all to update the generated OpenAPI files.

[cholmes]

@matthewhanson @m-mohr @anayeaye - could you all review this and let me know what you think? Seems like we should get this in before release, since we've not yet put out anything except rc1 that had our grouping overhaul. See #722 for the context / background on it.

[m-mohr]

Hmm, for me a RC is something that is basically feature-freeze except for bug fixes. This to me looks like a bit too much to change between RCs. On the other hand, I wouldn't want to change this again for 1.0 and the separation mostly seems to make sense.

Let's get through them one by one:

1. eo contains sun_azimuth and sun_elevation

That feels reasonable if there's no other data/extensions will be using these terms. SAR is not, but I can't tell whether other extensions would use it. Can anybody think of an upcoming extension that would also be related to sun angles? I'm especially thinking about Sentinel 3+, but most seems only EO related.

2. new extension os, Overhead Sensor for containing azimuth, off_nadir, and incidence

The prefix os seems not so intuitive and helpful. I'm thinking whether there's a better prefix that describes the fields better?

4. dropping redundant _angle suffix

Hmm, especially for incidence and azimuth I found the _angle prefix good in better describing the field. Incidence is a bit ambiguous (maybe only for me as a non-native speaker) and azimuth is just all over the place (azimuth[_angle], sun_azimuth[_angle], sar:(looks|pixel_spacing|resolution)_azimuth). So I think I'd prefer to leave the suffix in.

This PR HAS breaking changes so I unticked the box.
Also, this PR needs some more work. stac_extensions fields are often not updated for example.

Don't get me wrong, I really appreciate all your background work on this, @davidraleigh .

[davidraleigh]

The prefix os seems not so intuitive and helpful. I'm thinking whether there's a better prefix that describes the fields better?

Is it that the abbreviation makes it confusing? If it is that Overhead Sensor isn't a good field we could use Capture Angles?

Hmm, especially for incidence and azimuth I found the _angle prefix good in better describing the field. Incidence is a bit ambiguous (maybe only for me as a non-native speaker) and azimuth is just all over the place (azimuth[_angle], sun_azimuth[_angle], sar:(looks|pixel_spacing|resolution)_azimuth). So I think I'd prefer to leave the suffix in.

Azimuth, incidence and nadir as concepts are specifically about angles, so the _angle usage seems redundant.

This PR HAS breaking changes so I unticked the box.

Understood. I'd created an issue and made suggestions for changes, I didn't want someone else to have to slog through the work of making the changes.

Also, this PR needs some more work. stac_extensions fields are often not updated for example.

If you could point out to me the places where I need to fix it up, I'm happy to do the work.

Don't get me wrong, I really appreciate all your background work on this, @davidraleigh .

Absolutely @m-mohr ! I'm stoked to help get this sorted for 1.0 release

[matthewhanson]

I've added some comments here: #722 (comment)

Summarizing:

• I'm good with removing _angle if other people agree
• I still think all angles, including sun angles, belong in one extension

I like the name overhead_sensor for the extension, and the prefix os seems fine to me.

[cholmes]

@m-mohr - yeah, I'm conflicted on RC being 'bug fix', but I think from the standpoint of a user who is just using our stable releases 0.9.0 is going to be the first time they see a lot of changes in prefixes. Like I view all the common changes we've made as 'unreleased' as of yet. So it seems better to me to get the first release 'right', then to put out 0.9.0 and then turn around and do a 0.9.1 or wait till 1.0-beta to do these changes, if we agree they're the right one. And I'd guess I see release candidate as 'this is what we believe the release should be', and if the feedback is more than a bug fix we should change it.

As for specifics in this:

• I agree with @m-mohr on the os prefix not being great. The key point to me is that seeing a prefix like 'sat' or 'eo' you know what it is about. I'd interpret 'os' as 'open source' or 'ordnance survey' - though I think overhead sensor is the right name it is not a well known name / abbreviation. I'm not sure that I have any better idea, and I think it may be ok if it's just a less good prefix. One idea might be to use the prefix 'angles:' - a bit longer than normal, but then we could take off the suffixes. Perhaps then go with 'capture angles' for the prefix name.

• I did like the angles suffix, as it makes things quite clear. So I'd say keep it, unless we go with the angles prefix, in which case it'd be pretty redundant.

[cholmes]

And I'm nuetral on all angles being in one extension.

[m-mohr]

Okay, fine to do it for 0.9, @cholmes .

Is it that the abbreviation makes it confusing? If it is that Overhead Sensor isn't a good field we could use Capture Angles?

Overhead sensor in general seems to be about right for grouping the fields. It's just that "os" is a bit overloaded as Chris said and not self-explaining. "overhead_sensor" is probably too long and "sensor" too broad. So I don't have a better term yet, but I'm also not the best person to find one with my limited vocabulary. ;-)

In general, I'd like the prefixes to be more descriptive, for example we once had "dtr:datetime_start", which I'd have preferred to be "datetime:start". Then, when moving fields to core, you'd in general just need to replace the ":" with "_". I'm even not a fan anymore of my own "cube:dimensions" and think it should be "datacube:dimensions". We already have this problem with "eo", which is electro optical, but most people think "earth observation". It may have been better to use the "spectral" prefix or so. This would all lead to "nice" names when moving to core, e.g. spectral_bands. os_incidence on the other hand just doesn't explain itself very well; sensor:incidence_angle (or so) would be IMHO.
But overall that's a matter of taste and @matthewhanson for example preferrs shorter obviously.

I'm not a fan of using the "angle" prefix as it would limit the extensibility of the overhead sensor extension to just contain angles. Would be a different thing if we group all angles. I'm undecided whether that's a good idea or not.

Azimuth, incidence and nadir as concepts are specifically about angles, so the _angle usage seems redundant.

Only if you think about incidence in our context. In general, I'm thinking about software incidence management and other things first. It only really bugs me with incidence, the others are mostly fine as is although for SAR it could be a bit confusing with so many occurances of "azimuth". So I'm leaning towards adding _angles again. We don't really loose anything, just make it a bit more clear. Or is there any disadvantage in having them?

If you could point out to me the places where I need to fix it up, I'm happy to do the work.

In all example files you edited you need to check whether it needs to be changed. Also, you need to add the extension prefix to the JSON Schema for items. I'm just about to head home so don't have the time to request the changes explicitly now, but will see what's left to do the next day(s).

Thanks!

[matthewhanson]

The overhead sensor fields are really "viewing geometry", but that's not any shorter, and 'geometry' is way too overloaded to use.

What about "view". It would be a viewing extension, and provide fields such as view:sun_azimuth_angle.

[m-mohr]

Things to do:

1. Update stac_extensions for items (not collections), for example here:

   stac-spec/extensions/eo/examples/example-landsat8.json

   Line 4 in e6df7a1

   "eo",

   and probably in most of these files: https://github.com/radiantearth/stac-spec/tree/e6df7a1556342b766074f942abfd3bb71bbb3ab4/item-spec/examples
2. Update JSON Schema to include the new extension:

   stac-spec/item-spec/json-schema/item.json

   Line 72 in e6df7a1

   "checksum",

3. Similarly update the API spec and regenerate it later using npm:

   stac-spec/api-spec/openapi/STAC.yaml

   Line 741 in e6df7a1

   stac_extensions:

Hope it helps.

[davidraleigh]

@m-mohr @matthewhanson @cholmes
updated pull request. (I don't know Alex's github handle).

@m-mohr I made the changes, but I'm not sure I addressed what you pointed out before:

In all example files you edited you need to check whether it needs to be changed. Also, you need to add the extension prefix to the JSON Schema for items.

[davidraleigh]

Addressed items 1, 2, and 3 in #734 (comment)

[m-mohr]

Thanks @davidraleigh.

I checked it more precisely today, there's still files to update:

1. collection-spec\examples\landsat-item.json
2. commons\examples\landsat-item.json
3. 2nd and 3rd example in extensions\commons\README.md
4. Example in extensions\view\README.md
5. Same file and minor, but it says in the table for view:incidence_angle: "The incidence_angle angle is the angle". I'd remove the underscore and doubled "angle": "The incidence angle is the angle"
6. The JSON Schema still says: "Overhead Sensor Extension" in title and description.
7. There's no link from the extensions README to the new extension.
8. A separate full example in an example folder for the view extension is missing and would be great to have according to the extension guidelines. Maybe it's just moving one over from the item spec (there are quite a lot anyway). Or extend and move the one that is inline in the README.md.
9. You also still need to run npm run generate-all.

Unfortunately I can't push to your branch, otherwise I'd just committed the changes.

[davidraleigh]

@m-mohr I didn't want to add in any changes that I hadn't seen made when the sat extension was merged in. I'll go ahead and add the additional changes for stac_extensions and other changes now.

[m-mohr]

Yeah, it seems the sat PR was also a bit flawed, but a good time to fix things. Thanks!

[m-mohr]

Thanks for bearing with all my change requests. :-)

[davidraleigh]

@m-mohr the other pull request wasn't as flawed as the pull request I dropped in your lap. Thanks for putting your eye on it all and identifying the errors.

[cholmes]

Looks great, thanks for everyone's hard work on this! And especially @davidraleigh for driving it forward and @m-mohr for the detailed reviews.

I put in one very minor comment, questioning if we should recommend commons in the view extension. But no need to wait on that to merge.

[cholmes]

Very minor point, but on 'It is not necessary, but recommended to place common fields in [STAC Collections]' - won't most of these be changing if they are used? The sat extension orbit stuff I think is more likely to be static. But even planet has some variation in the view angles (not sure about landsat + sentinel, but I think they can?), and the sun angles will change with the time of year. So we can maybe get rid of this line.

[m-mohr]

Yes, true. I think we should encourage commons only for EO/SAR due to the bands fields.

[davidraleigh]

I think I just lifted this directly from the sat extension README.md

I can delete it.

[davidraleigh]

but maybe @matthewhanson has input on this line?

[davidraleigh]

nevermind :D