Add RasterBand::write_block
#490
Conversation
lnicola
force-pushed
the
rasterband-write-block
branch
2 times, most recently
from
b0b62e7 to
2fb4da8
Compare
|
Should we also support |
There was a problem hiding this comment.
I'm torn about having core operations that are only supported with in ndarray enabled, mode without a technical reason.
- Pros: easier to submit PRs.
- Cons: forces users to use ndarry when in reality
I'd like to use these features as a Buffer user.
But I admittedly have a bit of an axe to grind here. I would have preferred ndarray integration to have happened as a copy-free Buffer::to_ndarray method (for example), rather than having dual I/O methods for Buffer and for Array2.
| #[cfg_attr(docsrs, doc(cfg(feature = "array")))] | ||
| /// Write a [`Array2<T>`] from a [`Dataset`] block, where `T` implements [`GdalType`]. | ||
| /// | ||
| /// # Arguments |
There was a problem hiding this comment.
Can we add a reference to the method for getting the block layout (so one can know how many blocks there are in each dimension)?
There was a problem hiding this comment.
An example would be helpful here.
There was a problem hiding this comment.
Added an example and a note to block_size and actual_block_size.
| /// The Matrix shape is (rows, cols) and raster shape is (cols in x-axis, rows in y-axis). | ||
| pub fn write_block<T: Copy + GdalType>( | ||
| &mut self, | ||
| block_index: (usize, usize), |
There was a problem hiding this comment.
What happens if the block index is out of bounds? Do we get an error, or panic (or worse)?
There was a problem hiding this comment.
There's no way it can panic, of course :-). Added a note in the docs.
I think we should, but I don't think I want to do it in this PR, since there's a bit of design space to explore. I don't like the current situation either, but in the meanwhile it's probably better than having no access to these. |
lnicola
force-pushed
the
rasterband-write-block
branch
from
2fb4da8 to
5a8c2f2
Compare
|
@metasim do you ever use the MD API, and would you expect it to require |
@lnicola I do not. That's a good point (I didn't realize MD required it). For that API it seems reasonable to require I wouldn't be adverse to |
It doesn't, I should have checked before asking 😔. |
|
PS: I think |
CHANGES.mdif knowledge of this change could be valuable to users.