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

Python/fix/raster tests #452

Merged
merged 51 commits into from
Nov 10, 2023
Merged

Python/fix/raster tests #452

merged 51 commits into from
Nov 10, 2023

Conversation

sllynn
Copy link
Contributor

@sllynn sllynn commented Nov 8, 2023

adding tests for raster functions to python bindings

sllynn added 30 commits October 23, 2023 15:24
… into python/fix/gdal-enabler

bringing inline
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #452 (f5525c5) into main (ab7b5c7) will decrease coverage by 0.08%.
The diff coverage is 45.00%.

❗ Current head f5525c5 differs from pull request most recent head 0461940. Consider uploading reports for the commit 0461940 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #452      +/-   ##
==========================================
- Coverage   92.14%   92.07%   -0.08%     
==========================================
  Files         243      243              
  Lines        6671     6675       +4     
  Branches      234      233       -1     
==========================================
- Hits         6147     6146       -1     
- Misses        524      529       +5     
Files Coverage Δ
...ricks/labs/mosaic/core/crs/CRSBoundsProvider.scala 100.00% <100.00%> (ø)
.../labs/mosaic/core/raster/operator/CombineAVG.scala 100.00% <ø> (ø)
...ic/expressions/raster/RST_RasterToWorldCoord.scala 100.00% <100.00%> (ø)
...c/expressions/raster/RST_RasterToWorldCoordX.scala 100.00% <100.00%> (ø)
...c/expressions/raster/RST_RasterToWorldCoordY.scala 100.00% <100.00%> (ø)
...s/labs/mosaic/expressions/raster/RST_TryOpen.scala 83.33% <ø> (ø)
...ic/expressions/raster/RST_WorldToRasterCoord.scala 100.00% <100.00%> (ø)
...c/expressions/raster/RST_WorldToRasterCoordX.scala 100.00% <100.00%> (ø)
...c/expressions/raster/RST_WorldToRasterCoordY.scala 100.00% <100.00%> (ø)
...expressions/raster/base/Raster2ArgExpression.scala 100.00% <100.00%> (ø)
... and 5 more

@sllynn sllynn marked this pull request as ready for review November 9, 2023 14:22
@sllynn sllynn self-assigned this Nov 9, 2023
@@ -11,7 +11,7 @@ runs:
shell: bash
run: |
cd python
pip install build wheel pyspark==${{ matrix.spark }}
pip install build wheel pyspark==${{ matrix.spark }} numpy==${{ matrix.numpy }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we move up to Ubuntu 22.04 / GDAL 3.6.x we can pull this back out.

Copy link
Contributor

@milos-colic milos-colic left a comment

Choose a reason for hiding this comment

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

LGTM!

- name: Add packaged GDAL dependencies
shell: bash
run : |
sudo apt-get update && sudo apt-get install -y unixodbc libcurl3-gnutls libsnappy-dev libopenjp2-7
Copy link
Contributor

Choose a reason for hiding this comment

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

@sllynn this is so much better than before!

@@ -21,8 +21,8 @@ GDAL_RESOURCE_DIR=$(find $DATABRICKS_ROOT_VIRTUALENV_ENV -name "databricks-mosai

# -- untar files to root
# - from databricks-mosaic-gdal install dir
tar -xf $GDAL_RESOURCE_DIR/resources/gdal-3.4.3-filetree.tar.xz -C /
tar -xf $GDAL_RESOURCE_DIR/resources/gdal-3.4.3-filetree.tar.xz --skip-old-files -C /
Copy link
Contributor

Choose a reason for hiding this comment

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

@sllynn why is --skip-old-files needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running the script successfully is now part of the python test suite. The untar process is not idempotent and untar will raise an error if the target file(s) already exists. That's fine when running the script for the first time (on cluster start or in GH actions) but for local development you need to be able to re-run this multiple times without it hitting an error.

| div = np.sum(stacked_array > 0, axis=0)
| div = np.where(div==0, 1, div)
| np.divide(pixel_sum, div, out=out_ar, casting='unsafe')
| np.clip(out_ar, stacked_array.min(), stacked_array.max(), out=out_ar)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sllynn this was clever!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took forever to get it working using trial and error. Reading the docs helped a bit, mind you 🙄 Might be worth us building a little function that will allow us to pass in a script and inspect the internal state while it's executed?

@milos-colic milos-colic merged commit 3c5ab2e into main Nov 10, 2023
5 checks passed
sllynn pushed a commit that referenced this pull request Nov 20, 2023
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.

2 participants