Add driver for GeoDataFrame #750

Jaapel · 2024-01-24T16:11:58Z

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 one DataAdapter. 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 all DataAdapters. This class should be extendable to incorporate different ways of discovering data from a single URI.
I created a DataSource for loading items from the DataCatalog and verifying whether they are correct. These are specific for every DataAdapter.

Checklist

Updated tests or added new tests
Branch is up to date with v1
Tests & pre-commit hooks pass
Updated documentation if needed
Updated changelog.rst if needed
For predefined catalogs: update the catalog version in the file itself, the references in data/predefined_catalogs.yml, and the changelog in data/changelog.rst

Additional Notes (optional)

Add any additional notes or information that may be helpful.

savente93

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/data_adapter_v1/geodataframe_adapter.py

hydromt/typing.py

hydromt/data_sources/data_source.py

hydromt/metadata_resolvers/resolver_plugin.py

savente93 · 2024-01-26T12:41:41Z

hydromt/typing.py

+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]}'."
+


Do we want to validate a crs here too?

Perhaps we could make a BboxWithProjection type or something and use it.

tests/data_sources/test_geodataframe_data_source.py

tests/drivers/test_pyogrio_driver.py

tests/data_sources/test_geodataframe_data_source.py

Jaapel · 2024-02-08T14:46:30Z

@savente93 I notice that the tests are not passing due to "mamba not found" on windows test nodes. Any ideas?

savente93 · 2024-02-09T09:16:21Z

@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.

savente93

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/data_sources/geodataframe.py

hydromt/data_sources/data_source.py

hydromt/data_sources/geodataframe.py

savente93 · 2024-02-09T12:30:31Z

hydromt/drivers/pyogrio_driver.py

+
+from .geodataframe_driver import GeoDataFrameDriver
+
+GEOM_TYPES = Union[Geom, BaseGeometry]


move to typing/type_def.py please (or make a TODO for it)

savente93 · 2024-02-09T12:32:04Z

hydromt/gis/utils.py

+    geom: GPD_TYPES | None = None,
+    bbox: Bbox | None = None,
+    buffer: float = 0.0,
+    crs: CRS | None = None,


3.10 syntax, I'm surprised this got through the tests.

savente93 · 2024-02-09T12:33:48Z

hydromt/drivers/geodataframe_driver.py

+        crs: Optional[CRS] = None,
+        predicate: str = "intersects",
+        logger: Optional[Logger] = None,
+        # handle_nodata: NoDataStrategy = NoDataStrategy.RAISE,


should this still be commented out? if so is there a TODO for resolving this?

savente93 · 2024-02-09T12:34:57Z

hydromt/metadata_resolvers/convention_resolver.py

+        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 + "}"


Totally fine to do this later, but I'm wondeirng if we can replace this whole code block with a regex.

hydromt/metadata_resolvers/convention_resolver.py

savente93 · 2024-02-09T12:36:10Z

pyproject.toml

+markers = [
+  "integration: marks tests as being integration tests"
+]


do we also want one for unit tests?

savente93 · 2024-02-09T12:37:14Z

tests/conftest.py

+@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
+
+


Jaapel added 9 commits January 16, 2024 09:15

EOD

55db693

PlaceHolderURI

26c09a0

EOD

e3a1257

move crs and uri to methods

e8e4738

try metadata resolver

20bf0fb

EOD

fc8c09c

first test

b29e681

remove PlaceHolderURI

b76e56a

add Adapter

80bba63

Jaapel added GeoDataFrame issue related to GeoDataFrame adapter or processing V1 labels Jan 24, 2024

Jaapel self-assigned this Jan 24, 2024

test pyogrio driver

d03aa3c

savente93 reviewed Jan 26, 2024

View reviewed changes

Jaapel and others added 12 commits January 26, 2024 14:12

add unit tests

6ee6c43

add testing dependency

f7c0300

not use assert for pydantic validation

f5a268f

fix easy suggestions Sam

c1420ce

add generic constructor for data sources

b263687

update data adapters and catalog for v1

5953920

change typing back to 3.9

953f447

add marker for integration tests

d7148e0

enable manual debugging of testing GHA workflow

679f6e4

move data catalog tests

93152cb

do not keep v1 versions

6390ab1

Merge branch 'main' into add_driver

66720b3

Jaapel marked this pull request as ready for review February 7, 2024 16:10

Jaapel added 2 commits February 8, 2024 11:51

update data catalog parsing tests

110b398

skip tests that need further work

ab44bad

Jaapel requested a review from savente93 February 8, 2024 14:44

Jaapel requested a review from DirkEilander February 8, 2024 14:44

Jaapel added 2 commits February 9, 2024 11:48

Merge branch 'v1' into add_driver

3856c00

update changelog

2e64bd8

savente93 approved these changes Feb 9, 2024

View reviewed changes

Jaapel added 2 commits February 9, 2024 14:03

search for all >3.10 type hints

dfba974

use common typing

743e4b5

Jaapel merged commit cf65a1d into v1 Feb 9, 2024
4 of 8 checks passed

Jaapel deleted the add_driver branch February 9, 2024 13:33

savente93 linked an issue Apr 8, 2024 that may be closed by this pull request

Simplify data drivers #720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add driver for GeoDataFrame #750

Add driver for GeoDataFrame #750

Jaapel commented Jan 24, 2024 •

edited

Loading

savente93 left a comment

savente93 Jan 26, 2024

Jaapel Jan 26, 2024

Jaapel commented Feb 8, 2024

savente93 commented Feb 9, 2024

savente93 left a comment

savente93 Feb 9, 2024

savente93 Feb 9, 2024

savente93 Feb 9, 2024

savente93 Feb 9, 2024

savente93 Feb 9, 2024

Jaapel Feb 9, 2024

savente93 Feb 9, 2024


		from .geodataframe_driver import GeoDataFrameDriver

		GEOM_TYPES = Union[Geom, BaseGeometry]

Add driver for GeoDataFrame #750

Add driver for GeoDataFrame #750

Conversation

Jaapel commented Jan 24, 2024 • edited Loading

Issue addressed

Explanation

Checklist

Additional Notes (optional)

savente93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaapel commented Feb 8, 2024

savente93 commented Feb 9, 2024

savente93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaapel commented Jan 24, 2024 •

edited

Loading