set_geometry

[emanuel-schmid]

Changes proposed in this PR:

• use set_geometry in util.coordinates instead of direct column assignment

This PR fixes #888

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

[chahank]

Quick note: while the fix works, I think this is not a very forward-looking fix.

Currently, the method assumes that the inputs are geodataframes without a geometry column, but instead latitude and longitude columns. This is what we are trying to remove from the codebase (done in centroids, ongoing in exposures).

I suggest that we at least flag this method as one that should be refactored as soon as exposures have proper geodataframes.

[peanutfun]

You don't need a temporary column. set_geometry also accepts an array of values, and sets these as the geometry column, see https://geopandas.org/en/stable/docs/reference/api/geopandas.GeoDataFrame.set_geometry.html

[emanuel-schmid]

Er - are you sure? When running with a scheduler, dask.dataframe complains if there is no column to assign the values to. Isn't it?

[emanuel-schmid]

I suggest that we at least flag this method as one that should be refactored as soon as exposures have proper geodataframes.

No need to flag this. Any occurrence of geodataframes with latitude columns will eventually show up in the list of failed tests. 😁

[peanutfun]

When running with a scheduler, dask.dataframe complains if there is no column to assign the values to. Isn't it?

Ah, sorry. I have no idea about dask dataframes 🙈 For the single process using only GeoDataFrame, it should not be necessary.

[peanutfun]

Thanks for updating this! I think the CRS handling can be improved a bit.

However, I am wondering if the set_df_geometry_points needs the bit about the scheduler at all. geopandas.points_from_xy has become very efficient since Shapely v2.0, because the latter now uses multithreading. As the final geometry is anyway stored in a regular GeoDataFrame, and not in a distributed dask dataframe, I think we can safely remove the whole scheduler thing from set_df_geometry_points and make the function much simpler.

The linter complains anyway that the scheduler part is not tested 😅

[peanutfun]

Any GeoDataFrame has the attribute .crs. If it is not set, the attribute already returns None. So this can simply be

crs = df_val.crs if crs is None else crs  # crs might now still be None

[emanuel-schmid]

👍

[emanuel-schmid]

I think we can safely remove the whole scheduler thing from set_df_geometry_points and make the function much simpler.

@peanutfun : I'm inclined to believe you but would like to separate that from this PR, as it is not related to #888