Feature/reassign centr bydflt #527

emanuel-schmid · 2022-08-04T13:48:27Z

Changes proposed in this PR:

As discussed in PR Feature/refactor impact calc #436, not reassigning centroids may lead to faulty results through side effects
As decided on the CLIMADA developer day, the easiest solution is to introduce an ImpactCalc.impact parameter that may be used to suppress re-assignment of centroids
This PR proposes the parameter(s) reassign_centroids in both methods impact and insured_impact.
The parameter were set to True wherever it seemed suitable throughout the code base

This PR fixes issue #515

Pull Request Checklist

…roids(... overwrite=True)

set it to False wherever the assignment is obviously obsolete

peanutfun

Looking good! I mostly suggest cosmetic changes. In my suggestions for the comments I try to make clear to users what they gain by setting reassign_centroids to False, and when they can use it. Finally, by passing the force_reassign parameter to reassign_centroids, the method Forecast.calc can be simplified by quite a bit.

climada/engine/forecast.py

climada/engine/impact.py

climada/engine/impact_calc.py

climada/test/test_calibration.py

peanutfun · 2022-08-04T15:05:11Z

climada/engine/test/test_impact_calc.py

@@ -299,7 +299,7 @@ def test_calc_insured_impact_fail(self):
    def test_minimal_exp_gdf(self):
        """Test obtain minimal exposures gdf"""
        icalc = ImpactCalc(ENT.exposures, ENT.impact_funcs, HAZ)
-        exp_min_gdf = icalc.minimal_exp_gdf('impf_TC')
+        exp_min_gdf = icalc.minimal_exp_gdf('impf_TC', False)


Maybe make it explicit here as well?

Suggested change

exp_min_gdf = icalc.minimal_exp_gdf('impf_TC', False)

exp_min_gdf = icalc.minimal_exp_gdf('impf_TC', reassign_centroids=False)

peanutfun · 2022-08-04T15:14:50Z

Oh by the way, why is it called reassign_centroids and not simply assign_centroids? These methods behave exactly the same if centroids are assigned or not. So I think assign_centroids explains more exactly what's happening, and it's also equivalent to the Exposures.assign_centroids method. @chahank, @emanuel-schmid, what do you think?

chahank · 2022-08-04T22:05:59Z

I agree that the name should rather be assign_centroids. It is confusing to have reassign_centroids, when often there are no assigned centroids to begin with.

chahank · 2022-08-04T23:03:31Z

The methods in the module unsequa must also still be adjusted. I am not yet sure what is the right way to do it. In any case, the default should be to not reassign the centroids. The same is true for the cost_benefit module. It makes little sense to repeat this costly computation for repeated impact calculations for which the centroids do not change.

emanuel-schmid · 2022-08-05T08:19:39Z

If centroids are not assigned for that kind of hazard, they are going to be assigned regardless of the flag, hence, to me, reassign_centroids seems to be more accurate.

emanuel-schmid · 2022-08-05T08:32:56Z

the methods in the module unsequa must also still be adjusted. I am not yet sure what is the right way to do it. In any case, the default should be to not reassign the centroids. The same is true for the cost_benefit module. It makes little sense to repeat this costly computation for repeated impact calculations for which the centroids do not change.

You are right, I've noticed that too. And I'm not sure yet either about the right way. But in any case, I think the proposed changes here are independent and therefore follow-up PRs are the better way to deal with unsequa and cost_benefit.

peanutfun · 2022-08-05T08:52:35Z

If centroids are not assigned for that kind of hazard, they are going to be assigned regardless of the flag, hence, to me, reassign_centroids seems to be more accurate.

True, but I think this is unexpected with regard to the new method signature. If I set [re]assign_centroids=False, I would expect the method to never assign centroids. I think in this case the method should throw an error like

RuntimeError: '[re]assign_centroids' is set to 'False' but no centroids are assigned.

chahank · 2022-08-05T09:06:17Z

the methods in the module unsequa must also still be adjusted. I am not yet sure what is the right way to do it. In any case, the default should be to not reassign the centroids. The same is true for the cost_benefit module. It makes little sense to repeat this costly computation for repeated impact calculations for which the centroids do not change.

You are right, I've noticed that too. And I'm not sure yet either about the right way. But in any case, I think the proposed changes here are independent and therefore follow-up PRs are the better way to deal with unsequa and cost_benefit.

I disagree that this can be done separately. With the current change, we essentially make unsequa not usable, and cost benefit partially not usable.

…he Impact.calc method

Co-authored-by: Lukas Riedel <[email protected]>

consequently, don't assign if not set to true

do it manually before the method is called

peanutfun · 2022-08-05T12:27:42Z

climada/engine/impact_calc.py

+        assign_centroids : bool, optional
+            indicates whether centroids are re-assigned to the self.exposures object
+            or kept from previous impact calculation with a hazard of the same hazard type.
+            Centroids assignment is an expensive operation; set this to ``False`` to save 


I introduced trailing whitespace, which triggers new linter warnings. Sorry about that!

Suggested change

Centroids assignment is an expensive operation; set this to ``False`` to save

Centroids assignment is an expensive operation; set this to ``False`` to save

peanutfun · 2022-08-05T12:27:51Z

climada/engine/impact_calc.py

+        assign_centroids : bool, optional
+            indicates whether centroids are re-assigned to the self.exposures object
+            or kept from previous impact calculation with a hazard of the same hazard type.
+            Centroids assignment is an expensive operation; set this to ``False`` to save 


Suggested change

Centroids assignment is an expensive operation; set this to ``False`` to save

Centroids assignment is an expensive operation; set this to ``False`` to save

peanutfun · 2022-08-05T12:27:58Z

climada/engine/impact_calc.py

+        assign_centroids : bool
+            Indicates whether centroids are re-assigned to the self.exposures object
+            or kept from previous impact calculation with a hazard of the same hazard type.
+            Centroids assignment is an expensive operation; set this to ``False`` to save 


Suggested change

Centroids assignment is an expensive operation; set this to ``False`` to save

Centroids assignment is an expensive operation; set this to ``False`` to save

…c anymore

…pact.calc anymore

chahank · 2022-08-09T10:23:35Z

climada/engine/unsequa/calc_impact.py

+        exp.assign_centroids(haz, overwrite=False)
+        imp.calc(exposures=exp, impact_funcs=impf, hazard=haz, assign_centroids=False)


This is now a bit awkward, but it probably works fine. We should check whether the computation time remains stable. Have you already checked that on the test code? Shall I do some more testing?

I think the same change is needed for unsequa.calc_cost_benefit right?

Yes, same, same for calc_cost_benefit. I just checked it in.

And yes, I have checked the computational impact, not by computation time itself which is marginal for the test cases I can get my hands on, but by number of calls to assign_centroids with overwrite=True.

And no, thanks, that should be fine. You don't have to do more testing. Unless you object I'm going to merge.

analagous to the assignment policy in unsequa.calc_impact

emanuel-schmid added 4 commits July 20, 2022 17:59

cosmetics

e130f3d

impact_calc.ImpactCalc.minimal_exp_gdf: change default to assign_cent…

542aabf

…roids(... overwrite=True)

Merge branch 'develop' into feature/reassign_centr_bydflt

3f5e3ca

impact_calc: introduce optonal reassign_centroids argument

d7834e2

set it to False wherever the assignment is obviously obsolete

emanuel-schmid added the feature request label Aug 4, 2022

test_impact_calc: fix failing tests (due to reassignment)

50e6469

emanuel-schmid requested review from peanutfun and chahank August 4, 2022 14:11

peanutfun requested changes Aug 4, 2022

View reviewed changes

emanuel-schmid and others added 6 commits August 5, 2022 12:39

Merge branch 'develop' into feature/reassign_centr_bydflt

7fda806

forecast.Forecast.calc: simply pass the force_reassign parameter to t…

bfc94ae

…he Impact.calc method

Apply suggestions from code review

5df35b7

Co-authored-by: Lukas Riedel <[email protected]>

rename reassign_centroids arguments to assign_centroids

bad2513

consequently, don't assign if not set to true

calibration_opt: skip centroids assignment in calib_instance

9218ba0

do it manually before the method is called

fix typo

648c030

peanutfun reviewed Aug 5, 2022

View reviewed changes

emanuel-schmid added 5 commits August 5, 2022 15:12

PEP8 cosmetics

caf8dfd

forecast.calc: there is no implicit centroid assignment in impact.cal…

f0c609d

…c anymore

forecastforecast.calc: there is no implicit centroid assignment in im…

8ed6bbe

…pact.calc anymore

minimize centroid assignment in uncertainty, cost_benefit and measures

4834f79

measures.test: explicit centroid assignment before _cutoff

d764833

chahank reviewed Aug 9, 2022

View reviewed changes

emanuel-schmid added 2 commits August 9, 2022 16:30

unsequa.calc_cost_benefit: assign centroids manually, w/o overwriting

b7962ff

analagous to the assignment policy in unsequa.calc_impact

docstring consolidation

8c5ebe0

emanuel-schmid requested a review from peanutfun August 10, 2022 07:47

emanuel-schmid merged commit 7b634e8 into develop Aug 12, 2022

emanuel-schmid deleted the feature/reassign_centr_bydflt branch August 12, 2022 15:30

emanuel-schmid mentioned this pull request Aug 31, 2022

Make reassign_centroids the default in impact calculation #515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/reassign centr bydflt #527

Feature/reassign centr bydflt #527

emanuel-schmid commented Aug 4, 2022 •

edited

Loading

peanutfun left a comment

peanutfun Aug 4, 2022

peanutfun commented Aug 4, 2022

chahank commented Aug 4, 2022

chahank commented Aug 4, 2022 •

edited

Loading

emanuel-schmid commented Aug 5, 2022

emanuel-schmid commented Aug 5, 2022

peanutfun commented Aug 5, 2022

chahank commented Aug 5, 2022

peanutfun Aug 5, 2022

peanutfun Aug 5, 2022

peanutfun Aug 5, 2022

chahank Aug 9, 2022

emanuel-schmid Aug 9, 2022

	exp_min_gdf = icalc.minimal_exp_gdf('impf_TC', False)
	exp_min_gdf = icalc.minimal_exp_gdf('impf_TC', reassign_centroids=False)

	Centroids assignment is an expensive operation; set this to ``False`` to save
	Centroids assignment is an expensive operation; set this to ``False`` to save

		exp.assign_centroids(haz, overwrite=False)
		imp.calc(exposures=exp, impact_funcs=impf, hazard=haz, assign_centroids=False)

Feature/reassign centr bydflt #527

Feature/reassign centr bydflt #527

Conversation

emanuel-schmid commented Aug 4, 2022 • edited Loading

Pull Request Checklist

peanutfun left a comment

Choose a reason for hiding this comment

peanutfun Aug 4, 2022

Choose a reason for hiding this comment

peanutfun commented Aug 4, 2022

chahank commented Aug 4, 2022

chahank commented Aug 4, 2022 • edited Loading

emanuel-schmid commented Aug 5, 2022

emanuel-schmid commented Aug 5, 2022

peanutfun commented Aug 5, 2022

chahank commented Aug 5, 2022

peanutfun Aug 5, 2022

Choose a reason for hiding this comment

peanutfun Aug 5, 2022

Choose a reason for hiding this comment

peanutfun Aug 5, 2022

Choose a reason for hiding this comment

chahank Aug 9, 2022

Choose a reason for hiding this comment

emanuel-schmid Aug 9, 2022

Choose a reason for hiding this comment

emanuel-schmid commented Aug 4, 2022 •

edited

Loading

chahank commented Aug 4, 2022 •

edited

Loading