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

STACIT: STAC 1.1 support #10773

Closed
wants to merge 3 commits into from
Closed

STACIT: STAC 1.1 support #10773

wants to merge 3 commits into from

Conversation

m-mohr
Copy link
Contributor

@m-mohr m-mohr commented Sep 11, 2024

What does this PR do?

Implement support for STAC 1.1 in STACIT driver.

Is waiting for #10721

Disclaimer: Last time I wrote C/C++ was 10 years ago, this may take me a while...

What are related issues/pull requests?

None

Tasklist

  • Add support for STAC 1.1 in STACIT
  • Add support for STAC 1.1 in STACTA
  • Add support for STAC 1.1 in CLI (gdalinfo)?
  • Check common band names for validity
  • Make sure code is correctly formatted (cf pre-commit configuration)
  • Add test case(s)
  • Add documentation
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

@@ -139,8 +139,7 @@ int STACITDataset::Identify(GDALOpenInfo *poOpenInfo)
return pszHeader[0] == '{';
}

if (strstr(pszHeader, "\"stac_version\"") != nullptr &&
strstr(pszHeader, "\"proj:transform\"") != nullptr)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is proj:transform checked for? It could appear both in properties or assets so it seems like this check may lead to false results.

Copy link
Member

Choose a reason for hiding this comment

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

The driver requires the projection extension to be there, hence checking for it. And as Identify() needs to be quick (it only has a subset of the JSON and can't use a JSON parser), it doesn't get into the details to figure out where the element is present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, now I remember what strstr does. I'll revert, thanks.

Copy link
Contributor Author

@m-mohr m-mohr Sep 12, 2024

Choose a reason for hiding this comment

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

It needs two out of the three proj: properties and transform is one them. So just checking proj:transform seems to exclude cases where proj:bbox and proj:shape are set. Should we update this to check whether one of proj:transform or proj:bbox is present? That should capture all three cases.

fields currently new
transform + shape accepted accepted
transform + bbox accepted accepted
bbox + shape rejected accepted

Maybe worth a separate small PR though, it's unrelated to the STAC 1.1 updates.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth a separate small PR though, it's unrelated to the STAC 1.1 updates.

yes

Copy link
Member

Choose a reason for hiding this comment

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

taken into account per 8dd8c81

Comment on lines 614 to 615
else if (osCommonName == "alpha")
poVRTBand->SetColorInterpretation(GCI_AlphaBand);
Copy link
Contributor Author

@m-mohr m-mohr Sep 11, 2024

Choose a reason for hiding this comment

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

There is no common name called 'alpha" in STAC, thus removing.

Copy link
Member

Choose a reason for hiding this comment

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

Really ? but please don't remove that there, otherwise this is going to conflict with PR #10721 that touches that code too

Copy link
Contributor Author

@m-mohr m-mohr Sep 11, 2024

Choose a reason for hiding this comment

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

Yes, no alpha. The full list of allowed values is here: https://github.com/stac-extensions/eo?tab=readme-ov-file#common-band-names
So that code should never get executed.

Copy link
Contributor Author

@m-mohr m-mohr Sep 11, 2024

Choose a reason for hiding this comment

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

The list was also extended a bit recently and the property in STAC 1.1 is a named eo:common_name and can be placed in asset or bands. So I guess we need to do more updates in Gdal, including #10721

Copy link
Member

Choose a reason for hiding this comment

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

The list was also extended a bit recently

yes, #10721 takes inspiration from it, but feedback of the GDAL community was that some of the details like "rededge071", "rededge075", etc. were much too specific, so we didn't go to that detail for the GCI_ColorInterpretation enumeration.

I'm wondering if #10721 shouldn't be merge first (should be soon. I'm just letting a couple extra days for potential new comments on it), so that you can work on a stable base

Copy link
Contributor Author

@m-mohr m-mohr Sep 11, 2024

Choose a reason for hiding this comment

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

Yeah, go ahead. I can wait for that. This PR will still take me a little longer anyway.

@m-mohr m-mohr changed the title First attempt for STAC 1.1 support STACIT: STAC 1.1 support Sep 11, 2024
Co-authored-by: Even Rouault <even.rouault@spatialys.com>
@coveralls
Copy link
Collaborator

coveralls commented Sep 11, 2024

Coverage Status

coverage: 69.363% (+0.004%) from 69.359%
when pulling 166d5b3 on m-mohr:stac-1.1
into 5999d8b on OSGeo:master.

@rouault
Copy link
Member

rouault commented Sep 19, 2024

#10721 has been merged. Please rebase on top of latest master

Copy link

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@github-actions github-actions bot added the stale label Oct 18, 2024
@m-mohr
Copy link
Contributor Author

m-mohr commented Oct 18, 2024

Won't have time to work on this before December, but happy to reopen a new PR if this gets closed...

@stale stale bot removed the stale label Oct 18, 2024
Copy link

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@github-actions github-actions bot added the stale label Nov 16, 2024
@m-mohr
Copy link
Contributor Author

m-mohr commented Nov 16, 2024

Still not December ;)

@stale stale bot removed the stale label Nov 16, 2024
Copy link

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@m-mohr
Copy link
Contributor Author

m-mohr commented Dec 18, 2024

Closing for now, was a bit too optimistic about my time. If no one else works on it I may pick it up again at some point, others feel free to continue with the given code or start from scratch.

@m-mohr m-mohr closed this Dec 18, 2024
@rouault
Copy link
Member

rouault commented Jan 30, 2025

continued in #11760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants