Skip to content

Save file with driver COG#702

Merged
adebardo merged 4 commits into
GlacioHack:mainfrom
belletva:691-cog_format
Apr 16, 2025
Merged

Save file with driver COG#702
adebardo merged 4 commits into
GlacioHack:mainfrom
belletva:691-cog_format

Conversation

@belletva

@belletva belletva commented Mar 20, 2025

Copy link
Copy Markdown
Contributor

This PR adds the option to save file with driver COG in xdem/coreg/workflows.py.

Resolves #691.

@adebardo

Copy link
Copy Markdown
Contributor

I'm sorry, @belletva , but after discussing with @vschaffn , it might be more relevant to pass a string 'driver' str or None directly to dem_coregistration and then to save.

@belletva

Copy link
Copy Markdown
Contributor Author

@adebardo @vschaffn, in which cases the driver is None?

@adebardo

Copy link
Copy Markdown
Contributor

I meant that if it's None, then we have the default driver, sorry.

@adebardo

adebardo commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

Maybe driver and compression can be pass as kwargs ?
sorry for the changes

@belletva

Copy link
Copy Markdown
Contributor Author

Thanks for the comment @adebardo . However, the function dem_coregistration has already plenty of arguments. If we pass driver and compression as kwargs, maybe we should do the same for the other arguments? What do you think @rhugonnet and @adehecq ?

@rhugonnet

Copy link
Copy Markdown
Member

Thanks! My thoughts on this join this previous discussion: #650 (comment).
In short, I think we should deprecate dem_coregistration in favor of DEM.coregister_3d() (and very soon ds.dem.coregister_3d() through the Xarray accessor). 😅

This would solve the above problem by allowing to pass all arguments linked to I/O separately (which is the norm, whether in Xarray, Pandas or other):

dem1 = DEM(fn1)
dem2 = DEM(fn2)
# Case 1: In-memory
dem_coreg = dem1.coregister_3d(dem2)
dem_coreg.save(outfile, driver=, compression=)
# Case 2: Out-of-memory Multiprocessing
mp_config = MultiprocConfig(outfile, driver=, compression=)
dem_coreg = dem1.coregister_3d(dem2, mp_config=mp_config)
# Case 3: Out-of-memory Dask
dem_coreg = dem1.coregister_3d(dem2)
writer = dem_coreg.to_raster(outfile, encoding={driver=, compression=}, compute=False)
writer.compute()

I think having a one-liner convenience function that can take all these arguments at once would belong better in a CLI where we cannot chain I/O operations easily.
What do you think?

@rhugonnet

Copy link
Copy Markdown
Member

In the meantime, while we keep dem_coregistration a bit longer, we could simply add the arguments.

@adebardo

Copy link
Copy Markdown
Contributor

Thanks for your feedback, @rhugonnet .
You're right, I'll adjust my habits when using coregister_3d, it's important, so thanks for pointing it out.
I'm merging as-is for this one anyway.

@adebardo adebardo merged commit b255235 into GlacioHack:main Apr 16, 2025
@rhugonnet rhugonnet mentioned this pull request May 28, 2025
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.

COG format

3 participants