Avoid pickling shapely object in Exposures.write_hdf5

[emanuel-schmid]

Changes proposed in this PR:

• By default convert geometry data to wkb format before writing Exposures to HDF5 files.
• Add a flag to Exposures.write_hdf5 that allows to keep the previous behavior.
• Adapt the from_hdf5 methods accordingly.

The data is still be pickled but pickled byte arrays are unlikely to be subject to change.
This is not a performance boost. Quite likely there are faster ways to store geometry data as wkb arrays,
but they are not obvious and maybe hard to get right. In light of future plans to introduce the parquet file format as standard in climada, any more efforts to optimize have been omitted.

This PR fixes #

PR Author Checklist

• Read the Contribution Guide
• Correct target branch selected (if unsure, select develop)
• Descriptive pull request title added
• Source branch up-to-date with target branch
• Documentation updated
• Tests updated
• Tests passing
• No new linter issues
• Changelog updated

PR Reviewer Checklist

• Read the Contribution Guide
• CLIMADA Reviewer Checklist passed
• Tests passing
• No new linter issues

[peanutfun]

Thanks for the update! I am not sure that the current version actually converts the data to WKB, see my comment. Is there a particular reason why we need to keep the old behavior of write_hdf5 at all? We could simply switch to the new one, and avoid adding a new parameter (which will need to be deprecated at some point etc...)

[peanutfun]

The parameter name is very long and the fact that data is pickled internally might confuse users. How about

Suggested change

	def write_hdf5(self, file_name, pickle_geometry_as_shapely=False):
	def write_hdf5(self, file_name, geometry_to_wkb=True):

[peanutfun]

GeoSeries.to_wkb calls shapely.to_wkb on self. Are you sure this does what it's supposed to do?

Suggested change

	pandas_df[col] = gpd.GeoSeries.to_wkb(pandas_df[col])
	pandas_df[col] = gpd.GeoSeries(pandas_df[col]).to_wkb()

It might be more elegant to use GeoDataFrame.to_wkb on self, which already returns a pandas.DataFrame.

[emanuel-schmid]

Are you sure this does what it's supposed to do?

Sure! 😁 what makes you doubt it? Do you think we are missing a test?

[peanutfun]

Because you are calling a method of an uninstantiated object. Maybe use shapely.to_wkb(pandas_df[col]) directly?

[emanuel-schmid]

I'll keep it as it is (pandas_df[col] = gpd.GeoSeries(pandas_df[col]).to_wkb()) so geopandas can take care of it.

[emanuel-schmid]

Thanks for the review!

Is there a particular reason why we need to keep the old behavior of write_hdf5 at all?

Two reasons:

• I'm afraid the new way is slower, so the old way would be preferable if files don't have to be kept forever.
• For testing readability of old style files.

[peanutfun]

I'm afraid the new way is slower, so the old way would be preferable if files don't have to be kept forever.

If it's only a little, I would still be in favor of dropping it. If it's considerable, we need to tell that in the docs and probably change the default value?

For testing readability of old style files.

I only want to drop the support of writing old-style files. Reading should "silently" read both types.

[emanuel-schmid]

If it's only a little, ..

I've checked for Litpop from Russia: wkb is 5 (reading) to 10 (writing) times faster than shapely pickles.
So I agree, probably it's better to drop it.

For the test it's a pity though. Proof that the result is still the same. depends on a file that has been written with the deprecated method.
I'm tempted to skip that regression test as well and just trust the equality of the current implementation. 🤔

[emanuel-schmid]

I decided to skip tests for the old style files as well.
When all exposures from the data api are being replaced we have plenty of proof that reading old files is working and afterwards we don't need it anymore.

[peanutfun]

We can simplify this with GeoDataFrame.to_wkb():

wkb_columns = self.gdf.columns[self.gdf.dtypes == "geometry"].to_list()
pandas_df = self.gdf.to_wkb()  # Done!

See https://github.com/geopandas/geopandas/blob/7255c57371eafbff5a09f881db2b905d1f9b28a7/geopandas/geodataframe.py#L1254
(I cannot make this a suggestion on GitHub because there is a deleted line 🤷 )

[emanuel-schmid]

Hm. I thought I have checked that and found that gdf.to_wkb() only converts the 'geometry' column itself, leaving all other columns of type "geometry" untouched. Wrong?

[peanutfun]

Code and docs say it converts all geometry-type columns. The code I linked above is equivalent to our original one except for the explicit str cast on the returned dtype, which apparently is not necessary.

[peanutfun]

According to your test, it works fine, right?