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

Clarify EO items platform, constellation, and instrument #516

Merged
merged 17 commits into from Aug 8, 2019

Conversation

@philvarner
Copy link
Collaborator

commented Jun 25, 2019

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.
@@ -26,10 +26,10 @@ The exact metadata that would appear in a STAC Collection record will vary depen
| Field Name | Type | Description |
| ---------------- | ------------------------ | ----------- |
| eo:gsd | number | **REQUIRED.** Ground Sample Distance at the sensor. |
| 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 | Name of the constellation that the platform belongs to. See below for details. |

This comment has been minimized.

Copy link
@philvarner

philvarner Jun 25, 2019

Author Collaborator

moved so all REQUIRED are at top

@matthewhanson matthewhanson self-requested a review Jun 25, 2019
Copy link
Collaborator

left a comment

In general a good clarification, thanks. I added two minor comments for things that should be changed.

Also, I basically copy/pasted instrument, sensor and constellation from EO to SAR when I wrote the SAR extension some time ago. So, these clarifications should also be carried over to the SAR extension. Do you have time to do it, @philvarner? I'd only be able to do it in two weeks or so.

extensions/eo/README.md Outdated Show resolved Hide resolved
extensions/eo/README.md Outdated Show resolved Hide resolved
@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2019

@m-mohr I'm changing them to all be lowercase. I was reluctant to force instrument lowercase since all of them seem to be acronyms, but there should be not difference between that and a capitalized stylization.

extensions/sar/README.md Outdated Show resolved Hide resolved
extensions/sar/README.md Outdated Show resolved Hide resolved
philvarner added 2 commits Jul 24, 2019
@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 24, 2019

Wait, Sentinel 1 constists of 1A, 1B (at the moment), 1C and 1D (in the future)?!

Yeah, I realized that yesterday afternoon after i'd pushed the changes. Just fixed.

@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

@m-mohr I believe all of your concerns have been addressed now.

@m-mohr

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

@philvarner What benefits do you expect from recommending all lower-case and no spaces? I'm not convinced we need to recommend that only for this specific subset of fields. It makes the spec longer, but I don't see a big benefit. APIs should still work case-insensitive whenever possible as it is how user expect it IMHO and otherwise would make searching very hard. It would only be really helpful if it is actually required and can be checked with the validation tools. Also, all other text fields across STAC have no recommendation on casing.

philvarner added 3 commits Jul 31, 2019
@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 31, 2019

The recommendation is to reduce the cognitive load by attempting to have consistency among names. Otherwise, we'll get every variation, and that will make search more difficult. E.g., "landsat8" vs. "landsat_8", "landsat-8", "Landsat 8", "Landsat-8", "LANDSAT8", etc., or "landsat8" vs. "Sentinel-2 L2A"

@m-mohr

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

The recommendation is to reduce the cognitive load by attempting to have consistency among names. Otherwise, we'll get every variation, and that will make search more difficult. E.g., "landsat8" vs. "landsat_8", "landsat-8", "Landsat 8", "Landsat-8", "LANDSAT8", etc., or "landsat8" vs. "Sentinel-2 L2A"

@philvarner I agree that having an underscore or a space or a dash between name and number is a problem, but that's not solved by case sensitivity. There's still nothing in the spec regarding dash or underscore. Maybe we should even discourage dashes and underscores. Then we would just have landsat8 and sentinel2 in any casing, which I don't see as a big problem. Don't get me wrong, I'm fine with most of the changes in this PR, but I don't think case sensitivity is really a problem here, but the "special chars". So I am not sure how much value the recommendation regarding case sensitivity has and would rather not add it. That's just my opinion though.

@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

My view is that people are going to pick something arbitrary anyway, so we should have a lot of recommendations (but few required). I agree adding something about non-alphanumeric characters should be added.

@hgs-truthe01

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

Not sure if it is a complete fix, but I think this might be helped by the summary PR #527
That way it would be up to the catalog to list out valid strings.

Edit: @m-mohr I meant 527! thanks!

@cholmes
cholmes approved these changes Aug 1, 2019
philvarner and others added 2 commits Aug 1, 2019
@matthewhanson matthewhanson requested a review from m-mohr Aug 1, 2019
@matthewhanson matthewhanson removed the request for review from m-mohr Aug 1, 2019
@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

@m-mohr this is ready to merge -- your change request is blocking it, so please clear that. Thanks!

@m-mohr

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

@m-mohr this is ready to merge -- your change request is blocking it, so please clear that. Thanks!

So what was the conclusion on the issues I brought up, but couldn't discuss until the end?

  1. Why do we specify recommendations regarding case sensitivity for just those fields? Shouldn't we better add those to the best practices document? If casing is a problem in search (which I doubt), why not also recommend it for band name, collection keywords, provider names etc? I feel we make the field descriptions to long with all those recommendations.
  2. What about special characters, especially the mentioned - and _? You said that it leads different names such as Sentinel-2 and Sentinel_2. So Why not just recommend one of the characters instead of two?

Honestly, I'm not really happy about the recommendations yet. Either make strict recommendations or don't make any recommendations. Just some parts don't help very much, I think. Also, either make it for all identifier-like strings or none. Just picking some doesn't seem reasonable.

Edit: Oh, and I think @hgs-truthe01 wanted to link to #527 (or #413) instead of #416.

@cholmes

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Discussed on call 8/5. We want to move the special character / case recommendations to a 'best practices' document (evolve catalog one to be STAC), and put this info here, along with the recommendations in #464

@philvarner

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 7, 2019

Removed naming recommendations.

@m-mohr
m-mohr approved these changes Aug 8, 2019
@m-mohr m-mohr merged commit 1b8eb94 into radiantearth:dev Aug 8, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@m-mohr m-mohr deleted the philvarner:eo-clarifications branch Aug 8, 2019
@m-mohr m-mohr restored the philvarner:eo-clarifications branch Aug 8, 2019
@hgs-msmith hgs-msmith referenced this pull request Aug 20, 2019
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.