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.

Sign up for GitHub

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

Jump to bottom

jgrss/warp cleanup #306

Merged
merged 15 commits into from
Apr 26, 2024
Merged

jgrss/warp cleanup #306

merged 15 commits into from
Apr 26, 2024

Conversation

jgrss
Copy link
Owner

@jgrss jgrss commented Apr 20, 2024 •
edited
Loading

What is this PR changing?

  • Simplifies the mosaic() method and ensures support for multi-band mosaics
  • Fixes bug in to_raster(), where nodata values were not being written to file
  • Adds bigtiff keyword argument to save() method
  • Updates write tests

@jgrss jgrss requested a review from mmann1123 April 24, 2024 22:59
@jgrss
Copy link
Owner Author

jgrss commented Apr 24, 2024

@mmann1123 if you have time, could you test this branch out with some of your data?

@mmann1123
Copy link
Collaborator

Sure

mmann1123
mmann1123 previously approved these changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@mmann1123 mmann1123 left a comment

Choose a reason for hiding this comment

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

Looks good. Working on my end. I also added bigtiff to .save() please take a look. I added comments in the places you should look.

tests/test_write.py
@@ -96,63 +96,82 @@ def test_to_raster_multi(self):
# except FileExistsError:
# self.assertTrue(False)

def test_bigtiff(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jgrss new test

src/geowombat/core/io.py Outdated
@@ -786,7 +792,12 @@ def save(
compress=compress,
tiled=True if max(blockxsize, blockysize) >= 16 else False,
sharing=False,
bigtiff="IF_SAFER",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jgrss piling on here. I need to have bigtiff available for save

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an issue here trying it out on my files and I'm seeing empty rasters after mosaicing. Unfortunately I need to sign off and can't dig into why

@mmann1123 mmann1123 self-requested a review April 25, 2024 20:01
@mmann1123 mmann1123 dismissed their stale review April 25, 2024 20:04

There's a problem with mosaic

mmann1123
mmann1123 approved these changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@mmann1123 mmann1123 left a comment

Choose a reason for hiding this comment

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

I think the issues i was having had to do with hitting memory limits, mosaicing huge 18 band images must have been bonking it. I've tried to recreate the issues on reasonably large datasets and haven't had the same issue. I think this is working well for almost all purposes.

@mmann1123
Copy link
Collaborator

resolves #309

jgrss
jgrss commented Apr 26, 2024
View reviewed changes
src/geowombat/backends/xarray_.py
]

# Apply the reduction
darray = functools.reduce(
Copy link
Owner Author

Choose a reason for hiding this comment

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

@mmann1123 this is the main change for mosaicking.

jgrss
jgrss commented Apr 26, 2024
View reviewed changes
tests/test_write.py
self.assertTrue(data.equals(tmp_src))
# Compare attributes
self.assertTrue(
data.gw.nodataval == tmp_src.gw.nodataval == NODATA
Copy link
Owner Author

Choose a reason for hiding this comment

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

@mmann1123 new test to check that the nodata value was properly saved and read.

jgrss
jgrss commented Apr 26, 2024
View reviewed changes
src/geowombat/core/io.py
data.gw.nodata, float
):
kwargs['nodata'] = data.gw.nodata
if kwargs.get("nodata") is None:
Copy link
Owner Author

Choose a reason for hiding this comment

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

@mmann1123 this is the fix for ensuring the nodata value gets saved. Everything else in this file is auto-formatting.

@jgrss
Copy link
Owner Author

jgrss commented Apr 26, 2024

@mmann1123 I added a few more tests, which exposed a bug in to_raster(). Previously, the nodata value was not properly saved when using to_raster(). I commented on the fix in io.py.

jgrss
jgrss commented Apr 26, 2024
View reviewed changes
src/geowombat/core/geoxarray.py
@@ -739,6 +739,7 @@ def save(
num_workers: T.Optional[int] = 1,
log_progress: T.Optional[bool] = True,
tqdm_kwargs: T.Optional[dict] = None,
bigtiff: T.Optional[str] = None,
Copy link
Owner Author

Choose a reason for hiding this comment

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

@mmann1123 Does this work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

@jgrss
Copy link
Owner Author

jgrss commented Apr 26, 2024

@mmann1123 I made bigtiff a keyword argument, with a default of None. You should be able to pass data.gw.save(..., bigtiff='IF_SAFER'). Let me know if that doesn't work.

@mmann1123
Copy link
Collaborator

@mmann1123 I made bigtiff a keyword argument, with a default of None. You should be able to pass data.gw.save(..., bigtiff='IF_SAFER'). Let me know if that doesn't work.

That works.

@jgrss
Copy link
Owner Author

jgrss commented Apr 26, 2024

I think the issues i was having had to do with hitting memory limits, mosaicing huge 18 band images must have been bonking it. I've tried to recreate the issues on reasonably large datasets and haven't had the same issue. I think this is working well for almost all purposes.

Were you referring to this?

There's an issue here trying it out on my files and I'm seeing empty rasters after mosaicing. Unfortunately I need to sign off and can't dig into why

@jgrss
Copy link
Owner Author

jgrss commented Apr 26, 2024

We can postpone this merge if you want to diagnose the large raster mosaicking. Is the file too large to upload and share?

@mmann1123
Copy link
Collaborator

I think we merge this, but we can open a large mosaic issue.
I was able to resolve my issue using da.maximum() Maybe it is possible I was triggering compute or persist.

@jgrss jgrss marked this pull request as ready for review April 26, 2024 13:34
@jgrss jgrss merged commit 8c6cb0c into main Apr 26, 2024
3 checks passed
@jgrss jgrss deleted the jgrss/warp_cleanup branch April 26, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmann1123 mmann1123 mmann1123 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jgrss @mmann1123