Add option to pre-warp to a common projection #601

dmannarino · 2024-10-31T18:56:30Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
Check the commit's or even all commits' message styles matches our requested structure.
Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

Yes
No

Other information

…iner image)

codecov-commenter · 2024-10-31T20:18:36Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 43.47826% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.07%. Comparing base (7e9eef9) to head (989c14f).

Files with missing lines	Patch %	Lines
...s/raster_tile_set_assets/raster_tile_set_assets.py	46.66%	8 Missing ⚠️
app/tasks/raster_tile_set_assets/utils.py	28.57%	5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #601      +/-   ##
===========================================
- Coverage    81.23%   81.07%   -0.17%     
===========================================
  Files          130      130              
  Lines         5857     5876      +19     
===========================================
+ Hits          4758     4764       +6     
- Misses        1099     1112      +13

Flag	Coverage Δ
unittests	`81.07% <43.47%> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danscales

Looks good so far (assuming xargs parallelization works well), but as I comment below, don't you need a dependency so the unify_projection job finishes completely before running the pixetl job?

danscales · 2024-10-31T21:48:09Z

app/tasks/raster_tile_set_assets/raster_tile_set_assets.py

+
+    jobs.append(
+        await create_pixetl_job(
+            dataset, version, creation_options, "create_raster_tile_set", callback


***In the case the unify_projection is True, don't you need to specify a parent dependency argument here? The create_unify_projection_job must run fully before the create_pixetl_job runs.

Yep, thanks!

danscales · 2024-10-31T21:49:33Z

batch/scripts/unify_projection.sh

+  mkdir -p "$reprojected_dir"
+
+  cd $source_dir
+  for d in $((tree -dfi)); do


I think you mean $(tree -dfi) here (only one set of parens). Also, I guess we are sure the docker has the 'tree' command in it? You could use 'find' here as well.

I guess this may not matter right now, since our source URIs don't current have sub-directories of files.

I wasn't sure, I'll admit that was a try-it-and-see commit. It turns out we don't, and I've replaced the tree command with a find. Thanks for the parenthesis hint!

danscales · 2024-10-31T21:56:55Z

batch/scripts/unify_projection.sh

+  if [[ $s == gs://* ]]; then
+    gsutil cp -m -r "$s" "$source_dir"
+  elif [[ $s == s3://* ]]; then
+    aws s3 cp --no-progress "$s" "$source_dir"


I know we're not using aws for the source_uri, but shouldn't this have "--recursive" on it to copy an entire folder path and sub-folders?

Good catch, thanks!

dmannarino · 2024-11-01T03:47:33Z

Looks good so far (assuming xargs parallelization works well), but as I comment below, don't you need a dependency so the unify_projection job finishes completely before running the pixetl job?

You're right, thanks!

gtempus

I appreciate all the descriptions, @dmannarino . 👍
No tests? 🤷
raster_tile_set_asset is getting kinda long. Did you consider decomposing that function? Just curious.

danscales

Looks good! Let's get it pushed into staging and try it out on the dist alerts composites!

danscales · 2024-11-05T17:38:32Z

app/tasks/raster_tile_set_assets/raster_tile_set_assets.py

    callback: Callback = callback_constructor(asset_id)

-    create_raster_tile_set_job: Job = await create_pixetl_job(
-        dataset, version, creation_options, "create_raster_tile_set", callback
+    unify_job: Job | None = None


I think "Job | None" is more usually expressed as Optional[Job], probably more readable.

If you think it's more readable, I'll change it! It's true that it had to be expressed as you say until Python 3.10. This is my first time actually using the new syntax.

danscales · 2024-11-05T17:47:16Z

batch/scripts/_tiff_crosses_dateline.sh

+#
+# USAGE: ./crosses_dateline.sh infile [outfile]
+#
+# if no outfile is given, the script returns "true" or "false"


I think with the current modifications, there is no outfile arg, so maybe fix lines 6 and 8?

You're right, thanks!

danscales · 2024-11-05T17:59:33Z

app/tasks/raster_tile_set_assets/utils.py

+    callback: Callback
+):
+    """
+    """


Can you add a function comment here? Mainly would be that target_prefix is where the folders of warped files are placed, under SRC_0, SRC_1, SRC_2, etc.

Done, thanks!

dmannarino · 2024-11-05T20:54:57Z

I appreciate all the descriptions, @dmannarino . 👍 No tests? 🤷 raster_tile_set_asset is getting kinda long. Did you consider decomposing that function? Just curious.

True. Let me decompose and write a test as a follow-up.

solomon-negusse · 2024-11-07T15:16:41Z

batch/scripts/_tiff_crosses_dateline.sh

+
+crossing_dateline=false
+test $(python -c "print(${ulx}>${lrx})") = True && crossing_dateline=true
+test $(python -c "print(${ulx}>${lrx})") = True && crossing_dateline=true


@dmannarino: lines 37 and 38 look identical, just a duplicate or meant to check a different condition?

Oooh, good catch! It looks like I messed this up copying and pasting! They are meant to check a different condition. Will fix, thanks!

dmannarino added 13 commits October 29, 2024 14:50

Add unify_projection option, partially hardcoded MVP impl.

6b12cab

Whitespace breaks bash var assignments?

ba4a1a2

Run unify projection step in parallel

a5a5c8e

Fix expansion of CMD_ARGS

44540cb

Fix count/src_count mistake; insert possibly unnecessary brackets

a8b3d34

Echo commands for debugging; fix ref to wrong arg in helper

16a2cff

Fix counting srcs

686c871

Switch to GDAL2TilesJob for access to awscli (installed in that conta…

f8cafbd

…iner image)

Bump GDAL container version for gsutils

1b4dd60

Fix unify script by pre-making dirs

92b7c96

Fix only 1st src being used, I think

e311629

Allow for AWS source URIs

8690d5c

Removing time commands

6bbb9d0

dmannarino added 2 commits October 31, 2024 16:42

Change 'original' prefix to 'reprojected'

59e46d7

Recursively pre-dl files

5460f38

danscales reviewed Oct 31, 2024

View reviewed changes

dmannarino added 10 commits October 31, 2024 23:54

Fix gsutil command; fix aws dl command to be recursive

3455576

Set unify proj job to parent of pixetl job

6e6838a

Remove double parens

18fc7c8

Remove ./ from paths

71a3c2a

Replace tree (not in image) with find

d0c56d8

Fix grabbing 5 args with xargs when should have grabbed 4

10b7765

re-center TIFFs that cross the dateline

6634b8f

Possibly fix running _tiff_crosses_dateline.sh

98f4816

Replace bc with Python; hopefully fix gdalwarp arg complaints

e7614ae

Fix prefix name (epsg:4326 -> epsg-4326) to be consistent

459d90d

dmannarino changed the title ~~Add option to pre-warp to a common projection projection~~ Add option to pre-warp to a common projection Nov 3, 2024

dmannarino marked this pull request as ready for review November 5, 2024 15:42

dmannarino requested review from gtempus, solomon-negusse, danscales and manukala6 November 5, 2024 15:43

gtempus reviewed Nov 5, 2024

View reviewed changes

danscales approved these changes Nov 5, 2024

View reviewed changes

Improve docs in response to Dan's PR comments

c4b4a27

Merge branch 'develop' into add_unify_projection

989c14f

dmannarino merged commit 4dc4186 into develop Nov 5, 2024
2 checks passed

dmannarino deleted the add_unify_projection branch November 5, 2024 21:39

solomon-negusse reviewed Nov 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to pre-warp to a common projection #601

Add option to pre-warp to a common projection #601

dmannarino commented Oct 31, 2024 •

edited

Loading

codecov-commenter commented Oct 31, 2024 •

edited

Loading

danscales left a comment

danscales Oct 31, 2024

dmannarino Nov 1, 2024

danscales Oct 31, 2024

dmannarino Nov 3, 2024

danscales Oct 31, 2024

dmannarino Nov 1, 2024

dmannarino commented Nov 1, 2024

gtempus left a comment

danscales left a comment

danscales Nov 5, 2024

dmannarino Nov 5, 2024

danscales Nov 5, 2024

dmannarino Nov 5, 2024

danscales Nov 5, 2024

dmannarino Nov 5, 2024

dmannarino commented Nov 5, 2024

solomon-negusse Nov 7, 2024

dmannarino Nov 7, 2024

Add option to pre-warp to a common projection #601

Add option to pre-warp to a common projection #601

Conversation

dmannarino commented Oct 31, 2024 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

codecov-commenter commented Oct 31, 2024 • edited Loading

Codecov Report

danscales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmannarino commented Nov 1, 2024

gtempus left a comment

Choose a reason for hiding this comment

danscales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmannarino commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmannarino commented Oct 31, 2024 •

edited

Loading

codecov-commenter commented Oct 31, 2024 •

edited

Loading