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

update raster extension to v1.1.0 #809

Merged
merged 4 commits into from
Jun 19, 2022
Merged

Conversation

pjhartzell
Copy link
Collaborator

@pjhartzell pjhartzell commented May 16, 2022

Related Issue(s): #789

Description:

Makes "nan", "inf", and "-inf" valid values for the nodata property in raster:bands.

  • update schema uri
  • align the nodata type with the updated schema, which allows for numeric and "nan", "inf", and "-inf" strings.
  • update tests and test data. For the test data, the raster:bands object added to the B09 asset is from the sentinel example in the Raster Extension repo.

PR Checklist:

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

- update schema uri
- align nodata type with new schema: change nodata type from float to union of float and a StringEnum, where the StringEnum contains "nan", "inf", and "-inf"
- update tests
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #809 (31f072d) into main (1261e60) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #809   +/-   ##
=======================================
  Coverage   94.32%   94.33%           
=======================================
  Files          83       83           
  Lines       11885    11897   +12     
  Branches     1388     1389    +1     
=======================================
+ Hits        11211    11223   +12     
  Misses        495      495           
  Partials      179      179           
Impacted Files Coverage Δ
pystac/extensions/raster.py 89.92% <100.00%> (+0.14%) ⬆️
tests/extensions/test_raster.py 100.00% <100.00%> (ø)

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 1261e60...31f072d. Read the comment docs.

@pjhartzell pjhartzell marked this pull request as draft May 18, 2022 13:28
@pjhartzell
Copy link
Collaborator Author

Thanks @gadomski for pointing me at #448. Beyond the relevant discussion on how to handle extension updates in the future, it looks like an extension hook should be added to this PR to conform to the current method of handling extension version updates.

@pjhartzell
Copy link
Collaborator Author

In this case, the only change between v1.0.0 and v1.1.0 is the allowance of certain strings for the nodata value. So updating STAC from v1.0.0 to v1.1.0 only needs the schema URI in the extensions_list to change from v1.0.0 to v1.1.0.

However, the migrate method is only called when stac_version differs from the current value. So adding an extension hook only migrates the URI if the imported STAC is <1.0.0.

Copy link
Contributor

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

@pjhartzell Thanks so much for working on this, these changes all look good to me. Your comment on the migration only needing to update the schema URI is right on, so I think this is ready to be converted from Draft to PR and merged.

@duckontheweb
Copy link
Contributor

@pjhartzell Any objection to me converting this from a Draft?

@pjhartzell
Copy link
Collaborator Author

No objection to convert from Draft. Thanks for taking a look.

@duckontheweb duckontheweb marked this pull request as ready for review June 19, 2022 18:59
@duckontheweb duckontheweb merged commit a476084 into stac-utils:main Jun 19, 2022
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