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

Remove GDAL's Python bindings #222

Merged
merged 5 commits into from
Jan 20, 2022
Merged

Remove GDAL's Python bindings #222

merged 5 commits into from
Jan 20, 2022

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Jan 19, 2022

Related Issue(s): #216, #217

Description: To simplify our dependency tree, this PR removes direct use of the GDAL Python bindings (used in the addraster command). This represents a breaking change because it changes the function signature of cogify.

Additional fix:

cc @sgillies
Closes #216
Closes #217

PR checklist:

  • Code is formatted (run scripts/format).
  • Code lints properly (run scripts/lint).
  • Tests pass (run scripts/test).
  • Documentation has been updated to reflect changes, if applicable.
  • Changes are added to the CHANGELOG.

@gadomski gadomski added this to the v0.3.0 milestone Jan 19, 2022
@gadomski gadomski requested a review from duckontheweb January 19, 2022 17:02
gadomski added a commit that referenced this pull request Jan 19, 2022
gadomski added a commit that referenced this pull request Jan 19, 2022
@gadomski gadomski force-pushed the issues/216-remove-gdal branch from 8d3005d to 7ed4f7c Compare January 19, 2022 17:08
This changes the API of the cogify function.
This helps debug dict differences better.
This requires refactoring `add_raster_to_item` to use rasterio instead
of GDAL bindings. During this refactor, I discovered that the original
implementation was not correctly handling the top bound of the histogram
calculation, and so the last bin value was one too low in the tests. The
test case was corrected.
@gadomski gadomski force-pushed the issues/216-remove-gdal branch from 7ed4f7c to 988c691 Compare January 19, 2022 17:13
@gadomski gadomski changed the title Remove GDAL Remove GDAL's Python bindings Jan 19, 2022
@sgillies
Copy link

@gadomski I think the cogify function could be removed and left to rio-cogeo or gdal_translate, but this PR is fine. Rasterio's copy function calls GDALCreateCopy and thus should be a pretty complete COG maker.

Copy link

@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.

LGTM, I just had some comments around typing and possible parametrizing some tests. Nothing that should hold back this PR, though.

src/stactools/core/addraster.py Show resolved Hide resolved
src/stactools/core/addraster.py Show resolved Hide resolved
tests/cli/commands/test_addraster.py Show resolved Hide resolved
@gadomski gadomski merged commit 7a3a08f into main Jan 20, 2022
@gadomski gadomski deleted the issues/216-remove-gdal branch January 20, 2022 22:23
@gadomski gadomski mentioned this pull request Feb 15, 2022
5 tasks
@gadomski gadomski mentioned this pull request Mar 25, 2022
5 tasks
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.

Remove GDAL requirement
3 participants