Don't re-project projected polygon gdfs in metre-disaggregation

[Evelyn-M]

Changes proposed in this PR:

• When disaggregating a polygon-containing geodataframe, one needs to specify whether resolution is specified in metres. For gdfs which are already in a projected crs (aka a metre-based one), it is currently strictly necessary to give the resolution (in metres), yet indicate to_metres=False (since else the code tries to re-project the already projected gdf, and fails). This is however a bit confusing to some end-users.
• This hotfix checks in the mentioned case whether the gdf is already projected, and avoids re-projection.

This PR fixes #648

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 a lot for a concise pull request and for working through the checklist! 😍 The change appears very sensible to me but I have trouble understanding the test. Maybe you can walk me through and we might find a way to simplify it a bit

[peanutfun]

I really have trouble understanding this test 🙈 Can you walk me through what you are trying to test exactly?

I think one important check would be if _interp_one_poly is executed instead of _interp_one_poly_m when passing an Exposures object in a projected CRS. That one could be solved with mocking.

The final assert_allclose with an absolute tolernace of 100000 seems pointless to me. Can you explain why the tolerance is that large?

[Evelyn-M]

You're right, strictly speaking one should test that _interp_one_poly_m does not get called and _interp_one_poly is called instead in this example. But that surpassed my leisurely testing skills :D

I re-did this test now, just checking that the metre-based projection wasnt changed by the to_metres command (which indirectly checks this).

Not sure how mocking works, but could be an option for (future?) re-writes of the lines_polys_tests? It still has untested functions, like the ones you mentioned..

[Evelyn-M]

Anyways, this is not what's happening apparently (jenkins complains that the crs was overwritten to epsg:4326) - a behaviour which i cannot reproduce locally, where all the tests pass. will try to find out how this is possible

[peanutfun]

I can come up with a code that mocks of the _interp_one_poly[_m] methods. We can also discuss that in person.

[peanutfun]

@Evelyn-M done!

[peanutfun]

jenkins complains that the crs was overwritten to epsg:4326

@Evelyn-M This is probably because the code uses pd.concat to concatenate an empty (EPSG:4326) GeoDataFrame with the created one, and places it into a new GeoDataFrame. Either the empty frame overwrites the CRS of the created one, or the CRS is dropped along the way by pd.concat. See

climada_python/climada/util/lines_polys_handler.py

Line 525 in 85650dc

gdf_pnt = gpd.GeoDataFrame([])

and following lines

[chahank]

The bug was not in that line, but in the function swapping the geometry column. It happens that when one renames the geometry column, the crs information is lost.

Fixed in 87f2e8e

[chahank]

Additional info: line 525, which initializes an empty geodataframe does not fix a crs. When an empty geodataframe is created, not geometry column is defined, and thus no crs.

[Evelyn-M]

great, nice work. thanks both @chahank , @peanutfun !
I guess we can merge and close this?

[chahank]

Just one small note here: the reason why this check was previously not implemented is that when you set to_meters=True, each polygon is mapped onto an equal-area projection centered on the center of the polygon. Thus, if a user has polygons all over the world, the local projections help in minimizing the area errors.

Now, a projected crs does not necessarily preserve area, and if global, leads to local errors. With the proposed change, an imho important use case is thus now not possible anymore (i.e. reprojecting polygons from a projected crs to a local projected crs with area preservation).

[Evelyn-M]

Then you'd need to code up something for this usecase ;) at the moment, the search for the best-fitting metre-based projection expects a lat/lon based crs to start with. Hence the fail.

[chahank]

Then you'd need to code up something for this usecase ;) at the moment, the search for the best-fitting metre-based projection expects a lat/lon based crs to start with. Hence the fail.

aaa yeah, right, good point.