-
Notifications
You must be signed in to change notification settings - Fork 30
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
Pyogrio io #583
Pyogrio io #583
Conversation
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.
Thanks for this PR!
Generally it looks great and thanks again for checking all the different options I suggested :)
I have two final suggestions. Furthermore, I think it would be good if @savente93 could have a final look at the code syntax (I didn't pay much attention to this because of time).
hydromt/gis_utils.py
Outdated
@@ -553,3 +558,46 @@ def to_geographic_bbox(bbox, source_crs): | |||
bbox = Transformer.from_crs(source_crs, target_crs).transform_bounds(*bbox) | |||
|
|||
return bbox | |||
|
|||
|
|||
def prepare_pyogrio_reader_filters( |
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.
I think this method could be more widely used than for pyogrio input. Perhaps renaming it to parse_bbox
or something more generic would make that more clear.
pyproject.toml
Outdated
@@ -25,6 +25,7 @@ dependencies = [ | |||
"rioxarray", # wraps rasterio and xarray. Almost superceded by hydromt/raster.py | |||
"pandas", # Dataframes | |||
"pyflwdir>=0.5.4", # Hight models and derivatives | |||
"pyogrio>=0.6", # io for geopandas dataframes |
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.
Similar to other optional io packages I would suggest to move pyogrio to the optional io dependencies until it the default engine of geopandas.
I'll be reviewing it in a few minutes, but in the mean time I have one questuion:
I missed this, is it because of this issue, or because of something else? |
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.
Very nice PR Jaap, thanks. Two small questions but nothing major. Really nice.
Issue addressed
Fixes #575
Explanation
pyogrio
will be replacing geopandas read models in v1.0. This library does not try to doPath.expanduser
, so using this version fixes the bug for me. Masking still does not work in that version, so I had to do some work myself (filtering input / output).Checklist
main