ENH: optionally retrieve CRS from SRID in from_wkt/from_wkb #2956

jparta · 2023-07-11T07:18:08Z

This PR aims to enable reading geoalchemy WKTs and WKBs with SRID information in them. See documentation here: https://geoalchemy-2.readthedocs.io/en/latest/elements.html

wkb_or_wkb -> wkb_or_wkt

Convert elements to strings if needed

jparta · 2023-07-11T09:19:05Z

@martinfleis We have a dilemma here: either we have to devise a way to read the SRID from the raw data (hex or string), or convert the WKBElements into strings before passing them off to lib.from_wkb. Should an attempt be made to convert the elements to strings or bytes?

I solved it now by converting elements to string when needed. It's not very clean, though.

jparta · 2023-07-11T11:04:40Z

Now I've used the shapely get_srid method as suggested in #2952. EWKT reading is not working, with the following:

FAILED geopandas/tests/test_geoseries.py::TestSeries::test_from_ewkt - pygeos.GEOSException: ParseException: Unknown type: 'SRID=4326;POLYGON'
FAILED geopandas/tests/test_geoseries.py::TestSeries::test_from_ewkt_series - pygeos.GEOSException: ParseException: Unknown type: 'SRID=4326;POLYGON'

It seems GEOS is not able to read this format.
One option is to not do EWKT at all. Another is to remove the SRID header manually, which is slow and requires geopandas to keep track of possible changes to the standard.

What do you think?

martinfleis

Thanks! Some comments on the implementation side.

This will also need a changelog note.

martinfleis · 2023-07-15T14:44:17Z

geopandas/array.py

+ try:
+ len(data)
+ except TypeError:
+ raise TypeError(
+ "Input must be iterable (e.g. Series, list, array)"
+ f"found {type(data)} instead."
+ )


Use pandas.api.types.is_list_like instead.

martinfleis · 2023-07-15T14:54:12Z

geopandas/array.py

+ first_srid = srid_getter(data[0])
+ if all(first_srid == srid_getter(obj) for obj in data):
+ if first_srid > 0:
+ shared_crs = first_srid


I suppose you can do shapely.get_srid(data) to get an array of srids without a for loop on Python side, no? And then check they're all the same.

martinfleis · 2023-07-15T14:56:02Z

geopandas/array.py

+ "Geometry elements do not agree on CRS. This may cause problems. "
+ "CRS could not be inferred from data.\n",


Suggested change

"Geometry elements do not agree on CRS. This may cause problems. "

"CRS could not be inferred from data.\n",

"Geometries contain different SRIDs encoding different coordinate reference systems. "

"CRS could not be inferred from data.\n",

Something along these lines may be better?

martinfleis · 2023-07-15T14:59:58Z

geopandas/array.py

+ warnings.warn(
+ f"The elements passed share a CRS, but it does not match the "
+ "CRS passed as parameter.\n\n"
+ f"CRS passed as parameter: {param_crs}.\n"
+ f"CRS from data: {shared_crs}.\n"
+ "CRS could not be inferred from data.\n",
+ UserWarning,
+ stacklevel=4,
+ )


If a custom CRS is passed, we should not try to retrieve it from SRID to avoid calling get_srid when we don't need to. So that shall be added in the first place.

Then this check is not needed as we would go this path only if param_crs is None.

martinfleis · 2023-07-15T15:00:18Z

geopandas/array.py

@@ -174,13 +230,18 @@ def from_wkb(data, crs=None):
 ----------
 data : array-like
 list or array of WKB objects
+ The 'srid' can be read from geoalchemy2's WKBElement.


Suggested change

The 'srid' can be read from geoalchemy2's WKBElement.

martinfleis · 2023-07-15T15:01:33Z

geopandas/array.py

@@ -174,13 +230,18 @@ def from_wkb(data, crs=None):
 ----------
 data : array-like
 list or array of WKB objects
+ The 'srid' can be read from geoalchemy2's WKBElement.
 crs : value, optional
 Coordinate Reference System of the geometry objects. Can be anything accepted by
 :meth:`pyproj.CRS.from_user_input() <pyproj.crs.CRS.from_user_input>`,
 such as an authority string (eg "EPSG:4326") or a WKT string.


Suggested change

such as an authority string (eg "EPSG:4326") or a WKT string.

such as an authority string (eg "EPSG:4326") or a WKT string. If None, geopandas will try to infer CRS from the SRID of geometries (if available).

I would add this here, instead of that sentence above (note it will need better formatting to avoid linting fail).

martinfleis · 2023-07-15T15:04:22Z

geopandas/array.py

+ converted_data = vectorized.from_wkb(data)
+ shared_crs = _get_common_crs_from_geometries(converted_data, crs)
+ if shared_crs is not None:
+ crs = shared_crs
+ return GeometryArray(converted_data, crs=crs)


Suggested change

converted_data = vectorized.from_wkb(data)

shared_crs = _get_common_crs_from_geometries(converted_data, crs)

if shared_crs is not None:

crs = shared_crs

return GeometryArray(converted_data, crs=crs)

geoms = vectorized.from_wkb(data)

if not crs:

crs = _get_common_crs_from_geometries(geoms, crs)

return GeometryArray(geoms, crs=crs)

If _get_common_crs_from_geometries returns None when nothing useful is found, we can simplify it like this.

martinfleis · 2023-07-15T15:04:37Z

geopandas/array.py

@@ -200,12 +261,17 @@ def from_wkt(data, crs=None):
 ----------
 data : array-like
 list or array of WKT objects
+ The 'srid' can be read from geoalchemy2's WKTElement.


martinfleis · 2023-07-15T15:05:01Z

geopandas/array.py

+ converted_data = vectorized.from_wkt(data)
+ shared_crs = _get_common_crs_from_geometries(converted_data, crs)
+ if shared_crs is not None:
+ crs = shared_crs


as above. Note that you missed adapting return here.

martinfleis · 2023-07-15T15:08:20Z

Could you also check what is the performance hit on this one? I.e. how long does it take to get srids compared to the geometry decoding. We may want to add an option to completely avoid that path, even for crs=None.

martinfleis · 2023-07-15T15:10:31Z

geopandas/tests/test_geoseries.py

+ t1_wkb_elem = WKBElement(self.t1.wkb, srid=srid).as_ewkb()
+ sq_wkb_elem = WKBElement(self.sq.wkb, srid=srid).as_ewkb()


Can we not depend on geoalchemy2 in basic tests and depend on it only optionally when we want to test it explicitly? We don't have geoalchemy2 in most of our CI envs and I'd like to keep that like that. You can use shapely (set_srid) directly instead.

jparta added 2 commits July 11, 2023 10:11

Fix typo

65c1c17

wkb_or_wkb -> wkb_or_wkt

Get WKB/WKT CRS from SRID attribute

b9f3b31

Convert elements to strings if needed

jparta force-pushed the read_geoalchemy_WKBElement branch from 73e5748 to b9f3b31 Compare July 11, 2023 09:18

jparta mentioned this pull request Jul 11, 2023

ENH: The CRS available in Extended WKB (EWKB) is discarded in GeoSeries.from_wkb() #2952

Open

Use shapely get_srid

a84faa6

jparta marked this pull request as ready for review July 11, 2023 11:05

Check which backend is in use

eef7c48

martinfleis requested changes Jul 15, 2023

View reviewed changes

martinfleis changed the title ~~Read geoalchemy wkb / wkt element~~ ENH: optionally retrieve CRS from SRID in from_wkt/from_wkb Jul 15, 2023

martinfleis added this to the 0.14 milestone Jul 15, 2023

martinfleis added the enhancement label Jul 15, 2023

martinfleis reviewed Jul 15, 2023

View reviewed changes

martinfleis modified the milestones: 0.14, 1.0 Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: optionally retrieve CRS from SRID in from_wkt/from_wkb #2956

ENH: optionally retrieve CRS from SRID in from_wkt/from_wkb #2956

jparta commented Jul 11, 2023

jparta commented Jul 11, 2023

jparta commented Jul 11, 2023 •

edited

martinfleis left a comment

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis Jul 15, 2023

martinfleis commented Jul 15, 2023

martinfleis Jul 15, 2023

		"Geometry elements do not agree on CRS. This may cause problems. "
		"CRS could not be inferred from data.\n",

	such as an authority string (eg "EPSG:4326") or a WKT string.
	such as an authority string (eg "EPSG:4326") or a WKT string. If None, geopandas will try to infer CRS from the SRID of geometries (if available).

		t1_wkb_elem = WKBElement(self.t1.wkb, srid=srid).as_ewkb()
		sq_wkb_elem = WKBElement(self.sq.wkb, srid=srid).as_ewkb()

ENH: optionally retrieve CRS from SRID in from_wkt/from_wkb #2956

Are you sure you want to change the base?

ENH: optionally retrieve CRS from SRID in from_wkt/from_wkb #2956

Conversation

jparta commented Jul 11, 2023

jparta commented Jul 11, 2023

jparta commented Jul 11, 2023 • edited

martinfleis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinfleis commented Jul 15, 2023

Choose a reason for hiding this comment

jparta commented Jul 11, 2023 •

edited