set_geometry - Githubissues

CLIMADA-project / climada_python

Python (3.8+) version of CLIMADA

GNU General Public License v3.0

305 stars 119 forks source link

set_geometry #889

Closed emanuel-schmid closed 2 months ago

emanuel-schmid commented 3 months ago

Changes proposed in this PR:

use set_geometry in util.coordinates instead of direct column assignment

This PR fixes #888

PR Author Checklist

[x] Read the Contribution Guide
[x] Correct target branch selected (if unsure, select develop)
[x] Descriptive pull request title added
[x] 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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

emanuel-schmid commented 3 months ago

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