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

Fix issue in sentinel 2 metadata where 0 z coordinates are listed. #122

Merged
merged 2 commits into from
Jun 7, 2021

Conversation

lossyrob
Copy link
Member

@lossyrob lossyrob commented Jun 6, 2021

There are some product metadata files that contain 0 z coordinate values in the product metadata that is used to construct STAC Item geometries. This causes the Item geometry to be invalid. This change modifies the parsing logic to filter out '0' string values in the coordinates to avoid this issue.

There are some produt metadata files that contain 0 z coordinate
values in the product metadata that is used to construct STAC Item
geometries. This causes the Item geometry to be invalid. This change
modifies the parsing logic to filter out '0' string values in the
coordinates to avoid this issue.
@lossyrob lossyrob requested a review from gadomski June 6, 2021 18:32
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2021

Codecov Report

Merging #122 (6ebd129) into master (e1fcbff) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #122   +/-   ##
=======================================
  Coverage   85.26%   85.26%           
=======================================
  Files          82       82           
  Lines        2463     2463           
=======================================
  Hits         2100     2100           
  Misses        363      363           
Impacted Files Coverage Δ
..._sentinel2/stactools/sentinel2/product_metadata.py 91.01% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1fcbff...6ebd129. Read the comment docs.

Copy link
Member

@gadomski gadomski 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 as is, I have three questions mostly for my own education:

  • Is there chance of a boundary granule footprint that includes 0 as a x or y value that could get caught by the added check?
  • Are 3D geometries useful? Could this be solved by only supporting 2D?
  • Does one of the test files include the bogus z value?

@gadomski gadomski added this to the v0.1.6 milestone Jun 7, 2021
@gadomski gadomski added bug Something isn't working sentinel2 labels Jun 7, 2021
@lossyrob
Copy link
Member Author

lossyrob commented Jun 7, 2021

Is there chance of a boundary granule footprint that includes 0 as a x or y value that could get caught by the added check?

Looking at the precision of the other x/y values, I don't believe so, but could not guarantee it. I found it difficult to find documentation on this part of the sentinel product metadata, so there's some assumptions made here.

Are 3D geometries useful? Could this be solved by only supporting 2D?

The example I found had all 0 values for z, which is common in a bug I've seen elsewhere where the default z values are printed out unintentionally. I have a hunch that a similar issue happened here with some scenes printing the default. I couldn't find documentation of meaningful Z footprint values.

Does one of the test files include the bogus z value?

The added test case reads an example product metadata file that was found to have the bogus Z values.

@gadomski
Copy link
Member

gadomski commented Jun 7, 2021

Roger, that all sounds good to me. Once CI passes I can merge. Thanks!

@gadomski gadomski merged commit 065ff38 into master Jun 7, 2021
@gadomski gadomski deleted the fix/rde/sentinel-2-z-values branch June 7, 2021 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants