added assign_centroids for impact objects by timschmi95 · Pull Request #602

added assign_centroids for impact objects by timschmi95 · Pull Request #602 · CLIMADA-project/climada_python

Changes proposed in this PR:

added util function assign_gdf_centroids to assign hazard centroids to any geodataframe
wrapper for impact object to assign hazard centroids to impact objects

This PR supports the calibration of impact functions based with damage data from impact objects

PR Author Checklist

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

PR Reviewer Checklist

The code is quite readable.

I think it can be merged after a more thorough look at the doc string (I pushed few suggestions and made few comments).

Also, it'd be good to have a coupe of tests.

@aleeciu I added a test for the imp.assign_centroids() method, and all tests are passing. Shall I covert it to a 'real' pull request?

Thanks a lot! The time for refactoring centroids assignment is just right I think. 😁
I suggest to first change the signature and behavior of the util.coordinates.assign_haz_centroids function and add some unittests that take its whole argument space into account.

@emanuel-schmid I think everything is resolved now. Do you want to have a last look over it before I merge it?

Comment on lines +1766 to +1768

		#Assert that the imp crs is epsg:4326, as it is required by the u_coord methods
		if not u_coord.equal_crs(self.crs, 'EPSG:4326'):
		raise ValueError('Set Impact to lat/lon crs (EPSG:4326)!')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question (sorry 😁) - which u_coord methods require epsg:4326?
And why can't they speak for themselves, i.e. raise an exception if the crs is wrong?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this in there as afterwards I label the coordinate column 'latitude' and 'longitude', but you are right: as with exposures the functionalities also work with other CRS, it's just the column labels 'latitude'/'longitude' that are not correct then, but this shouldn't be a problem. I think we can savely remove this part.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no real assignment anymore, the method name is kind of misleading.
My favorite would be closest_centroids. But then, the same argument is also valid for the coordinates methods assign_centroids_to_gdf, assign_gridpoints and assign_coordinates. Consequently they should be renamed too. Therefore I propose a wimp compromise: assigned_centroids as in "if there was an assignment the assigned centroids would be assigned like this:"

And CHANGELOG.md needs to be updated ... 😁

		fake_aai_agg = np.sum(fake_eai_exp)
		imp = Impact.from_eih(exp, ENT.impact_funcs, HAZ,
		fake_at_event, fake_eai_exp, fake_aai_agg)
		imp_centr = imp.assign_centroids(HAZ)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	imp_centr = imp.assign_centroids(HAZ)
	imp_centr = imp.assigned_centroids(HAZ)

Adding a method assigned_centroids sounds like a terrible idea to me, sorry. This makes just a very confusing naming. What is the purpose of this new method?

The information about the closest centroid from a hazard of an impact object is already in the exposures at the moment of computation.

I really begin to be confused about the purpose of this pull request.

With regard to Impact.assign_centroids - it's for those cases where you still have the impact, but not the exposures anymore I reckon. (?)
Apart from the Impact.asssign_centroids method it's a nice decomposition of Exposures.assign_centroids.

@timschmi95 : could you please clarify the use case of the methods impact.assign_centroids ?

@chahank The impact.assign_centroids is used for the spatially explicit calibration, which we do for hail damages. I.e. the reported damages are read into an impact object, and assigned hazard intensity values for each reported impact allow for a spatially explicit calibration.

It could also be used by if someone is just interested in what hazard intensity led to which (modelled) impacts. Ofc if the imp is produced with ImpactCalc().impact() one could also use the exp.assign_centroids() method instead, but the impact.assign_centroids woud be more direct

@emanuel-schmid I am also not sure about the proposed name impact.assigned_centroids.
As you pointed out with the newest change the assigned centroids are not stored in the impact object anymore. Nevertheless, they are assigned to each impact coordinate point (just not stored within the imp object), thus I would vote to keep the name as impact.assign_centroids. Or if preferred I think impact.closest_centroids also works well, even if within the function it then uses the ``assign_centroids_to_gdf``` function.

Many thanks @timschmi95
I've changed all the misleading assign_ names to match_ (with the exception of Exposures.assign_centroids, of course) and simplified the Impact.match_centroids method somewhat.
Now I'm happy. If @chahank approves, we can merge.

Thanks @emanuel-schmid the new names are also fine with me! And the use of _build_exp() also makes the method shorter :)

I still think that there should not be any method impact.macht_centroids. Since it is a single liner and it was only introduced to be used in the calibration module, I would remove it entirely. The pull request was nevertheless very useful as the utility methods were much improved.

That being said, if you really think it is useful, please feel free to merge.

timschmi95 deleted the feature/assign_centroids_impact branch

March 23, 2023 20:27