-
Notifications
You must be signed in to change notification settings - Fork 20
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
Subscript-crop Raster
or Vector
by bracket call []
, consistency and bug fixes
#335
Conversation
…ke projtools independent of georaster and geovector, and add tests
[]
, consistency and bug fixes
[]
, consistency and bug fixes[]
, consistency and bug fixes
[]
, consistency and bug fixesRaster
or Vector
by bracket call []
, consistency and bug fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Romain!
These additions are awesome. You're really contributing to this being maintainable in the long term! On the subject of maintainability, I have some comments on details that I struggle to understand, but generally, it's great!
Regarding your two suggestions about the subscripting:
- On the first, do you mean to slice by indices like
arr.isel()
ordf.iloc[]
in xarray and pandas, respectively? If so, it would actually be pretty cool, but I struggle to see a use case. For iterating along blocks perhaps? Or subsetting for debug purposes? Those two are good arguments in my head, but for the amount of complexity they introduce, it might not be a great idea unless there are more uses. - I personally think the subscripting syntax should only use crop. Reproject can be very cpu-intensive and should in my opinion always be explicit. Cropping, however, is really fast, so there the subscripting syntax makes a ton of sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to make crop more consistent and thanks for fixing all the small issues.
The brackets are a nice feature, but they would need to be documented at some point, otherwise, we'll just forget that it's possible !
Thanks a lot! All updated, merging 😄 |
Summary
This PR updates the
get_geoprojected_bounds
function, moving its core toprojtools
to make it consistent and accessible for bothRaster
andVector
classes.Then, it adds the same options in
Vector.crop()
than forRaster.crop()
(= passing anotherRaster
,Vector
or a list of bounds).Finally, it implements the
__getitem__
(=[]
) method forRaster
andVector
, which calls their underlyingcrop
functionality (= subsetting), as suggested previously by @erikmannerfelt.Other than that, several bug fixes and test additions.
Discussion on the
__getitem__
implementationSeveral things:
slice
objects, which would then be used onself.data
forRaster
(subsettingnp.MaskedArray
in 2D) andself.ds
onVector
(subsettinggpd.GeoDataFrame
in 1D).reproject
instead ofcrop
when it isRaster[Raster]
(only one where it makes sense)? The resampling algorithm could be passed as second argumentRaster[Raster, "bilinear"]
.Related issues and more details
This PR:
Vector.create_mask()
only withxres
argument #276,Raster.crop
does not reproject the bounding box of a Raster object provided ascropGeom
#262,Raster
dtypes #333.Also it:
projtools.py
onraster.py
andgeovector.py
,get_geoprojected_bounds
function inRaster
toprojtools.py
,get_geoprojected_bounds
as a method for theVector
class,Raster
class forVector
objects (can provide any type of geometry: bounds,Raster
,Vector
),Raster.polygonize()
,intersection
triggered by package updates.