-
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
Add driver for GeoDataFrame #750
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.
Very nice work Jaap, I think the proof of concept looks really nice. I left a lot of comments, many of them pedantic, but they're just suggestions. In general I think this is going to a really good place!
hydromt/typing.py
Outdated
def _validate_bbox( | ||
bbox: tuple[float, float, float, float], | ||
) -> tuple[float, float, float, float]: | ||
assert ( | ||
bbox[0] < bbox[2] | ||
), f"bbox minx: '{bbox[0]}' should be less than maxx: '{bbox[2]}'." | ||
assert ( | ||
bbox[1] < bbox[3] | ||
), f"bbox miny: '{bbox[1]}' should be less than maxy: '{bbox[3]}'." | ||
|
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.
Do we want to validate a crs here too?
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.
Perhaps we could make a BboxWithProjection
type or something and use it.
@savente93 I notice that the tests are not passing due to "mamba not found" on windows test nodes. Any ideas? |
@Jaapel yeah, I still haven't had time to properly configure the windows tests, that's one me. You can ignore that until I fixed that. |
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 work Jaap. I didn't look through al lthe tests in detail, but I'm very impressed by them. I think the real proff of the pudding will be in the integration, so for now I think this is great. I left a couple of small comments, but overal I'm really liking the direction this is going in!
hydromt/drivers/pyogrio_driver.py
Outdated
|
||
from .geodataframe_driver import GeoDataFrameDriver | ||
|
||
GEOM_TYPES = Union[Geom, BaseGeometry] |
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.
move to typing/type_def.py please (or make a TODO for it)
hydromt/gis/utils.py
Outdated
geom: GPD_TYPES | None = None, | ||
bbox: Bbox | None = None, | ||
buffer: float = 0.0, | ||
crs: CRS | None = None, |
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.
3.10 syntax, I'm surprised this got through the tests.
crs: Optional[CRS] = None, | ||
predicate: str = "intersects", | ||
logger: Optional[Logger] = None, | ||
# handle_nodata: NoDataStrategy = NoDataStrategy.RAISE, |
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.
should this still be commented out? if so is there a TODO for resolving this?
if "{" in uri: | ||
uri_expanded = "" | ||
for literal_text, key, fmt, _ in Formatter().parse(uri): | ||
uri_expanded += literal_text | ||
if key is None: | ||
continue | ||
key_str = "{" + f"{key}:{fmt}" + "}" if fmt else "{" + key + "}" |
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.
Totally fine to do this later, but I'm wondeirng if we can replace this whole code block with a regex.
markers = [ | ||
"integration: marks tests as being integration tests" | ||
] |
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.
do we also want one for unit tests?
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.
We could!
@pytest.fixture() | ||
def mock_driver(geodf: gpd.GeoDataFrame) -> GeoDataFrameDriver: | ||
class MockGeoDataFrameDriver(GeoDataFrameDriver): | ||
def read(self, *args, **kwargs) -> gpd.GeoDataFrame: | ||
return geodf | ||
|
||
driver = MockGeoDataFrameDriver.model_validate({}) | ||
return driver | ||
|
||
|
||
@pytest.fixture() | ||
def mock_resolver() -> MetaDataResolver: | ||
class MockMetaDataResolver(MetaDataResolver): | ||
def resolve(self, uri, *args, **kwargs): | ||
return [uri] | ||
|
||
resolver = MockMetaDataResolver.model_validate({}) | ||
return resolver | ||
|
||
|
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!
Issue addressed
Adresses for GeoDataFrame #720
Explanation
Since we want to work with multiple sources of data, we seperate functionality that the DataAdapter class was responsible for and split it until multiple classes. I implemented this just for the
GeoDataFrame
data type within HydroMT for a single driver (PyogrioDriver
).I created a
Driver
class for loading the data. This class is specific for oneDataAdapter
. This class should be extendable for plugins and data sources that we do not want to maintain (like custom APIs).I created a
MetaDataResolver
class for resolving the URI (think naming convention, but later also a STAC catalog or a GeoNetwork). This resolver is generic for allDataAdapters
. This class should be extendable to incorporate different ways of discovering data from a single URI.I created a
DataSource
for loading items from theDataCatalog
and verifying whether they are correct. These are specific for everyDataAdapter
.Checklist
v1
Additional Notes (optional)
Add any additional notes or information that may be helpful.