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

feat: add GDALRasterizeGeometries wrapper #213

Merged
merged 10 commits into from
Sep 21, 2021

Conversation

msalib
Copy link
Contributor

@msalib msalib commented Sep 1, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This new function rasters::rasterize lets you "burn in" a sequence of vector::Geometry objects into a raster Dataset.

@lnicola
Copy link
Member

lnicola commented Sep 3, 2021

I haven't looked too closely, but it feels like RasterizeOptions should have a manual Default implementation. There's no obvious reason why MergeAlgorithm should default to Replace, so that decision should be left for RasterizeOptions to take.

@msalib
Copy link
Contributor Author

msalib commented Sep 3, 2021

Ah, that makes more sense. Done!

@msalib
Copy link
Contributor Author

msalib commented Sep 3, 2021

I'm not 100% certain where the best place to put this is. I considered making it a Dataset method, but it only makes sense for raster Datasets, and it seems a bit niche. It might also go in the vector module since it takes a sequence of Geometry objects but that also seems odd.

I'm also not 100% convinced that passing in separate slices for geometries and burn_values is the best; it might make more sense to have a struct or pair with both and just pass a slice of that struct/tuple. But doing that might be annoying for cases where folks have an expensive to compute collection of Geometrys and want to pass different burn_values without updating. This might be better structured as a builder.

@lnicola
Copy link
Member

lnicola commented Sep 15, 2021

Sorry for taking so long on this. I don't mind where the code sits, but I'm not too fond of that TextOptions API, mostly because it stores two Vecs. Other places call CSLSetNameValue directly, maybe you could do the same until we get a better API.

@msalib
Copy link
Contributor Author

msalib commented Sep 16, 2021

No worries on the delay; everyone is busy and I really appreciate your review!

This version uses a TextOptions that stores only a pointer and calls CSLSetNameValue directly as you suggested.

There are a few places that use asserts for input validation; would you prefer that I replace those with real errors added to GdalError instead?

@msalib
Copy link
Contributor Author

msalib commented Sep 16, 2021

Actually, the asserts were bothering me, so I just put in more proper error handling, but I'm happy to remove it if you prefer.

@lnicola
Copy link
Member

lnicola commented Sep 16, 2021

This version uses a TextOptions that stores only a pointer and calls CSLSetNameValue directly as you suggested.

Yeah, that looks better. I'm not too keen on the name, but I don't have any great suggestion (CSLStringList?) and it's not public yet.

Actually, the asserts were bothering me, so I just put in more proper error handling, but I'm happy to remove it if you prefer.

The error on special characters is kinda' fine, though I'm not sure it's really required. GDAL doesn't do anything fancy there, I think their point is simply to not have the separator in the key.

The other checks are less idiomatic: the user gave us some bogus arguments that don't make sense. It's closer to "used the wrong index in an array" than "a file was missing from disk". To put it differently, if you made this mistake in your code, would you prefer a panic right off the bat, or an error that was propagated up the stack until handled somewhere?

But I'm around "-0" on these, and feel free to ignore my opinion anyway (or to wait/ask for another one).

@msalib
Copy link
Contributor Author

msalib commented Sep 20, 2021

Sorry for my confusion, but do you want any other changes?

@lnicola
Copy link
Member

lnicola commented Sep 20, 2021

Ah, no, I don't have any other strong feelings about the panics, but maybe someone else wants to take a look?

@lnicola
Copy link
Member

lnicola commented Sep 21, 2021

bors r+

I guess we can still iterate on this if anything comes up.

@bors
Copy link
Contributor

bors bot commented Sep 21, 2021

@bors bors bot merged commit 4593766 into georust:master Sep 21, 2021
@msalib msalib deleted the feature/add-GDALRasterizeGeometries branch September 22, 2021 23:23
This was referenced Oct 13, 2021
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