Improve impact calc error messages when there is a mismatch of impf ids between exposures and impfset by luseverin · Pull Request #863 · CLIMADA-project/climada_python

luseverin · 2024-03-22T16:51:29Z

Changes proposed in this PR:

Add a test in ImpactCalc.impact(exposures, impfset, hazard) to handle AttributeError arising when an impact function id contained in the exposures has no matching impact function in the impact function set.
Change previous AttributeError: 'list' has no attribute 'calc_mdr' to a more explanatory error message

This PR fixes #669 (reopened)

PR Author Checklist

PR Reviewer Checklist

…pfset in ImpactCalc.impact()

…pfset and exposures in ImpactCalc.impact()

…set and exposures in ImpactCalc.impact()

chahank

Thanks for the quick fix! I have some very small style requests.

Also, please do not forget to complete the changelog and to add yourself to the author's list

chahank · 2024-03-25T07:12:21Z

climada/engine/impact_calc.py

        impf_col = self.exposures.get_impf_column(self.hazard.haz_type)
+
+        # check for compability of impact function id between impact function set and exposure
+        if any(if_id not in self.impfset.get_ids(haz_type=self.hazard.haz_type) for


Please use the similar variable names as in the rest of the file: if -> impf.

Besides, a variable name if is rather confusing as this is also a python operator. Note that we had a longer discussion on that 2 years ago when we decided to avoid the variable name if.

Thanks for the notice, I changed if_id to impf_id.

chahank · 2024-03-25T07:15:59Z

climada/engine/test/test_impact_calc.py

        except Exception as e:
            self.assertEqual(str(e), "Impact calculation not possible. No impact "
                             "functions found for hazard type TC in impf_set.")
+    def test_error_handling_mismatch_impf_ids(self):


If being pedantic, I would improve slightly the test by having two impact functions in the set, one that is in the exposures, and one that isn't.

Ok so now the test considers the case of having two impact functions in the exposures (impf ids 1 and 2), and two impact functions in the impfset (impf ids 1 and 3), so that an error is raised as there is no impact function with id 2 in the impfset. Let me know in case you meant something else.

chahank · 2024-03-25T07:16:42Z

climada/engine/test/test_impact_calc.py

+            self.assertEqual(str(e),  "At least one impact function associated to the exposures has no match in the impact function set.\n"
+                "The impact functions in the impact function set for hazard type \"TC\" have ids [2]. "
+                "The column \"impf_TC\" in the exposures contains the ids [1].\n" 
+                "Please make sure that all exposure points are associated with an impact function that is included in the impact function set.")


The linter should complain that these lines are too long. Could you please shorten them?

Sorry I might have missed this. I think it is good now.

…est_error_handling_mismatch_impf_ids

luseverin · 2024-03-25T14:59:10Z

Thanks for the review! I think I have adressed all your comments. I also modified the author's list and changelog. Let me know if there's anything else.

chahank · 2024-03-25T15:22:12Z

climada/engine/test/test_impact_calc.py

+        impf_exp = ImpactFunc()
+        impf_exp.id = 1
+        impf_exp.intensity = np.array([0, 20])
+        impf_exp.paa = np.array([0, 1])
+        impf_exp.mdd = np.array([0, 0.5])
+        impf_exp.haz_type = 'TC'
+        impf_noexp = deepcopy(impf_exp)
+        impf_noexp.id = 3


This should work too right? Ideally, one does not first initiate an object and then modify its attributes. This is mostly a Matlab-style remnant in older CLIMADA code.

Suggested change

impf_exp = ImpactFunc()

impf_exp.id = 1

impf_exp.intensity = np.array([0, 20])

impf_exp.paa = np.array([0, 1])

impf_exp.mdd = np.array([0, 0.5])

impf_exp.haz_type = 'TC'

impf_noexp = deepcopy(impf_exp)

impf_noexp.id = 3

impf_exp = ImpactFunc(haz_type='TC', id=1)

impf_noexp = deepcopy(impf_exp)

impf_noexp.id = 3

Yes that's indeed better, thanks for the suggestion!

chahank · 2024-03-25T15:22:16Z

climada/engine/test/test_impact_calc.py

+        try:
+            ImpactCalc(exp, impfset, haz).impact()
+        except Exception as e:
+            self.assertEqual(str(e),  "At least one impact function associated to"
+                             " the exposures has no match in the impact function set.\n"
+                "The impact functions in the impact function set for hazard type \"TC\" "
+                "have ids [1, 3]. The column \"impf_TC\" in the exposures contains the ids"
+                " [1, 2].\nPlease make sure that all exposure points are associated with an "
+                "impact function that is included in the impact function set.")


Sorry, I did not see that first. For the unit test, please use self.assertRaises() : https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises

Yes that makes sense to use this instead. I modified the testing function.

emanuel-schmid · 2024-03-25T15:35:31Z

climada/engine/impact_calc.py


        impf_col = self.exposures.get_impf_column(self.hazard.haz_type)
+
+        # check for compability of impact function id between impact function set and exposure


Suggested change

# check for compability of impact function id between impact function set and exposure

# check for compatibility of impact function id between impact function set and exposure

Thanks for pointing out the typo!

emanuel-schmid · 2024-03-25T16:11:58Z

climada/engine/impact_calc.py

+                "At least one impact function associated to the exposures has no match "
+                "in the impact function set.\nThe impact functions in the impact function"
+                f" set for hazard type \"{self.hazard.haz_type}\" have ids "
+                f"{self.impfset.get_ids(haz_type=self.hazard.haz_type)}. The column "
+                f"\"{impf_col}\" in the exposures contains the ids "
+                f"{np.unique(self.exposures.gdf[impf_col].values).astype(int).tolist()}.\n"
+                "Please make sure that all exposure points are associated with an impact "
+                "function that is included in the impact function set.")


while we're at it why not just list the felonies:

Suggested change

"At least one impact function associated to the exposures has no match "

"in the impact function set.\nThe impact functions in the impact function"

f" set for hazard type \"{self.hazard.haz_type}\" have ids "

f"{self.impfset.get_ids(haz_type=self.hazard.haz_type)}. The column "

f"\"{impf_col}\" in the exposures contains the ids "

f"{np.unique(self.exposures.gdf[impf_col].values).astype(int).tolist()}.\n"

"Please make sure that all exposure points are associated with an impact "

"function that is included in the impact function set.")

unknown_impact_functions = list(self.exposures.gdf[

~self.exposures.gdf[impf_col].isin(known_impact_functions)

][impf_col].drop_duplicates().astype(int))

raise ValueError(

f"The associated impact function(s) {', '.join(unknown_impact_functions)}"

" have no match in impact function set for hazard type '{self.hazard.haz_type}'\n"

"Please make sure that all exposure points are associated with an impact "

"function that is included in the impact function set.")

Great, that's even better this way! I have included this modification in my last commits.

emanuel-schmid · 2024-03-25T16:12:00Z

climada/engine/impact_calc.py

+        if any(impf_id not in self.impfset.get_ids(haz_type=self.hazard.haz_type) for
+               impf_id in np.unique(self.exposures.gdf[impf_col].values)):


or maybe -

Suggested change

if any(impf_id not in self.impfset.get_ids(haz_type=self.hazard.haz_type) for

impf_id in np.unique(self.exposures.gdf[impf_col].values)):

known_impact_functions = self.impfset.get_ids(haz_type=self.hazard.haz_type)

if not all(self.exposures.gdf[impf_col].isin(known_impact_functions)):

which is possibly easier to read

Yes that is more readable, I also included this. Thanks!

emanuel-schmid · 2024-03-25T16:14:53Z

Good catch!

…g impfs

chahank · 2024-04-08T14:00:59Z

The compatibility test with petals is failing, but I think it is unrelated to this PR. @emanuel-schmid @luseverin are we ready to merge?

luseverin · 2024-04-08T14:11:21Z

It also does not seem to me that the tests failing are related to the PR. So ready to merge from my side.

emanuel-schmid · 2024-04-09T06:19:06Z

Thanks a lot!

…ds between exposures and impfset (CLIMADA-project#863) * Add error handling for missmatching impf ids between exposures and impfset in ImpactCalc.impact() * Clarify error message for handling of mismatching impf ids between impfset and exposures in ImpactCalc.impact() * Add unit test for error handling of mismatchin impf ids between impf set and exposures in ImpactCalc.impact() * Make code compliant with PEP8 * Change ambiguous variable names * Add impact function in the impf set but not in the exposure for the test_error_handling_mismatch_impf_ids * Add name to authors's list * Add changes to changelog * Use assertRaises instead of assertEqual to test the error handling * Refine error message so that it provides users with the ids of missing impfs * Initiate dummy impf in testing function directly setting its attributes * Correct for pylint warnings for too long lines --------- Co-authored-by: luseverin <[email protected]>

luseverin added 5 commits March 22, 2024 08:55

Add error handling for missmatching impf ids between exposures and im…

e50e0c2

…pfset in ImpactCalc.impact()

Clarify error message for handling of mismatching impf ids between im…

f78df73

…pfset and exposures in ImpactCalc.impact()

Add unit test for error handling of mismatchin impf ids between impf …

e677042

…set and exposures in ImpactCalc.impact()

Merge branch 'develop' into feature/improve_impact_calc_error_messages2

3067426

Make code compliant with PEP8

d01162c

luseverin added enhancement accepting pull request Contribute by raising a pull request to resolve this issue! labels Mar 22, 2024

luseverin assigned chahank and leonie-villiger Mar 22, 2024

luseverin removed the accepting pull request Contribute by raising a pull request to resolve this issue! label Mar 22, 2024

chahank requested changes Mar 25, 2024

View reviewed changes

luseverin added 4 commits March 25, 2024 15:03

Change ambiguous variable names

eaf6adf

Add impact function in the impf set but not in the exposure for the t…

5f5727a

…est_error_handling_mismatch_impf_ids

Add name to authors's list

97dfd12

Add changes to changelog

c48fc8c

chahank reviewed Mar 25, 2024

View reviewed changes

emanuel-schmid reviewed Mar 25, 2024

View reviewed changes

luseverin added 4 commits March 25, 2024 17:26

Use assertRaises instead of assertEqual to test the error handling

ed3d76f

Refine error message so that it provides users with the ids of missin…

a41a6a2

…g impfs

Initiate dummy impf in testing function directly setting its attributes

afe8d42

Correct for pylint warnings for too long lines

4e85009

emanuel-schmid merged commit 7f74ea4 into develop Apr 9, 2024

emanuel-schmid deleted the feature/improve_impact_calc_error_messages2 branch April 9, 2024 06:19

peanutfun mentioned this pull request Aug 9, 2024

Improve impact calc error messages #669

Closed


		impf_col = self.exposures.get_impf_column(self.hazard.haz_type)

		# check for compability of impact function id between impact function set and exposure

	# check for compability of impact function id between impact function set and exposure
	# check for compatibility of impact function id between impact function set and exposure

		if any(impf_id not in self.impfset.get_ids(haz_type=self.hazard.haz_type) for
		impf_id in np.unique(self.exposures.gdf[impf_col].values)):

Conversation

luseverin commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Author Checklist

PR Reviewer Checklist

Uh oh!

chahank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luseverin commented Mar 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emanuel-schmid commented Mar 25, 2024

Uh oh!

chahank commented Apr 8, 2024

Uh oh!

luseverin commented Apr 8, 2024

Uh oh!

emanuel-schmid commented Apr 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

luseverin commented Mar 22, 2024 •

edited

Loading