Feature/add missing jrc flood impact functions #111

aleeciu · 2024-02-06T09:37:30Z

This PR adds the missing flood impact fucntions from the JRC publications. The current implementation only uses the "residential buildings" functions, and this PR adds those from the five more categories (or sectors as tjey are called here) available in the JRC report, i.e., commercial buildings, industrial buildings, transport, infrastructure and agriculture. To do that, I had to slightly adjust the code as one can no longer only choose the region, but both the region and a sector.

chahank · 2024-04-08T12:13:22Z

climada_petals/entity/impact_funcs/test/test_fl.py

@@ -35,116 +35,114 @@ def test_flood_imp_func_set(self):

    def test_region_Africa(self):


Would it not be simpler to have tests that loop through the allowed parameters and all methods? This class just stores data, so this is what should be tested. Also, a test for when wrong parameters are used would be good. This makes the test more complete, and also shorter.

I used the backbone structure of the already existing tests and adapted them to the changes of provided by this new PR. If you think tests are poorly written, I would open a new PR which rewrites them.

That would be a good idea. The code base is constantly improving, and thus copying older, less mature code is suboptimal going forward.

chahank · 2024-04-29T14:43:37Z

@aleeciu : could you please provide a timeline to finish this PR?

aleeciu · 2024-05-03T10:22:03Z

Apologies, I replied quite timely in fact, but I did not realize I had to click on "send the review" in order for my comment to be visible. Anyway, here you really find me in line with comments on tests in CLIMADA-project/climada_python#734 : I am surprised about the request because the logic I followed is just the one already in the module so I find writing better tests very very important but simply beyond scope, and increasing scope is how one never gets things done :-)

…tions

aleeciu · 2024-08-07T15:56:54Z

I added tests looping through all possible combinations while catching wrong combinations. I also added test for wrong value assignments. In my view this is all it was asked in the review. Please let me know.

chahank

Excellent! Please just update the code to use init methods properly.

For future pull request it might be beneficial to write new quality code following the guidelines instead of copying legacy code.

Please also update the CHANGELOG file so that these changes are recorded for the next release.

Otherwise, concerning the current style of the methods with various if/else statements, I would appreciate confirmation from @emanuel-schmid that it is fine to leave it as is.

chahank · 2024-08-16T12:10:51Z

climada_petals/entity/impact_funcs/river_flood.py

+        impf = cls()
+        impf.name = f"Flood {region} JRC {sector.capitalize()} noPAA"
+        impf.continent = f"{region}"
+        impf.id = impf_id
+        impf.mdd = impf_values
+        impf.mdr = impf_values
+        impf.intensity = np.array([0., 0.05, 0.5, 1., 1.5, 2., 3., 4., 5., 6., 12.])
        impf.paa = np.ones(len(impf.intensity))
+
        return impf


Please use the init of the class properly. The init variables are:

id: Union[str, int] = "", intensity: Optional[np.ndarray] = None, mdd: Optional[np.ndarray] = None, paa: Optional[np.ndarray] = None, continent: str = '' name: str = "" So, the code should be return cls( id = impf_id, mdd = impf_values, paa = np.ones(len(intensity)), intensity = intensity, name = name, continent=continent )

Also, ImpactFunc does not have an attribute .mdr. Please make sure to remove this.

chahank · 2024-08-16T12:14:53Z

climada_petals/entity/impact_funcs/river_flood.py

+        if sector == 'residential':
+            impf_values, impf_id = from_jrc_impf_residential(region)
+
+        elif sector == 'industrial':
+            impf_values, impf_id = from_jrc_impf_industrial(region)
+
+        elif sector == 'commercial':
+            impf_values, impf_id = from_jrc_impf_commercial(region)
+
+        elif sector == 'transport':
+            impf_values, impf_id = from_jrc_impf_transport(region)
+
+        elif sector == 'infrastructure':
+            impf_values, impf_id = from_jrc_impf_infrastructure(region)
+
+        elif sector == 'agriculture':
+            impf_values, impf_id = from_jrc_impf_agriculture(region)
+


Long chains of if and else statements are to be avoided. For future work please use a dataclass or similar.

chahank · 2024-08-16T12:16:34Z

climada_petals/entity/impact_funcs/river_flood.py

+
+def flood_imp_func_set(sector="residential"):
+    """Builds impact function set for river flood, using standard files. By default, it reads
+    functions for the residential sector"""

    impf_set = ImpactFuncSet()


Please use a proper initialization. Something like:

impf_Set = ImapctFuncSet([
impf_africa,
impf_asia,
...
])

adjusted as suggested

chahank · 2024-08-16T12:20:09Z

climada_petals/entity/impact_funcs/river_flood.py

+                       "Use ImpfRiverFlood.from_region_sector instead.")
+        self.__dict__ = ImpfRiverFlood.from_jrc_region_sector("SouthAmerica", *args, **kwargs).__dict__
+
+def from_jrc_impf_residential(region):


This would probably have been much better solved with a dataclass. However, I am not sure it is worth the effort now. @emanuel-schmid what do you think?

aleeciu · 2024-09-16T16:21:05Z

Excellent! Please just update the code to use init methods properly.

For future pull request it might be beneficial to write new quality code following the guidelines instead of copying legacy code.

Please also update the CHANGELOG file so that these changes are recorded for the next release.

Otherwise, concerning the current style of the methods with various if/else statements, I would appreciate confirmation from @emanuel-schmid that it is fine to leave it as is.

Thanks a lot. An update:

I will change the method initiation
Concerning legacy code and following guidelines: I must say I read the guidelines but, unless I am reading the wrong ones, they are much less descriptive than what you say they are :-)
Waiting for @emanuel-schmid to give green light on current status or refactoring is needed

…tions

emanuel-schmid · 2024-09-27T16:20:05Z

Actually no, I couldn't overcome myself and give green light. 😬
Instead I committed some beautifications. Please have a look @aleeciu. If you think it's still ok, you're welcome to merge.

aleeciu · 2024-10-29T16:14:59Z

I committed a couple of changes to address the remaining issues from @chahank . Thanks @emanuel-schmid for your beautfications :-)

I would merge if it wasn't for these test errors, which are however - I think - unrelated to this PR? At least I don't have them when I run tests locally.

Feel free to merge or let me know how I can fix these tests failures.

Thanks for your review.

…tions

emanuel-schmid · 2024-10-31T10:03:27Z

@aleeciu the failing test is actually related to this pr: before there were 86 lines covered and 16 missed, now there are 79 lines covered and 15 missed, which is a slightly lower ratio. Just because you made the code denser, so to say. 🤷
We can safely ignore it.

emanuel-schmid · 2024-11-01T15:53:39Z

@aleciu can you please fix the failing integration test: climada_petals.test.test_flood_integr.TestRiverFlood.test_full_impact and submit a new PR?

One problem is the DEMO exposures has 3 for each impf_RF value, while the ids from flood_imp_func_set() has id's [11, 21, 31, 41, 51, 61].

The other problem is that the result differs significantlly:

test_flood_integr.py", line 80, in test_full_impact
    self.assertAlmostEqual(imp.at_event[0], 226839.72426476143)
AssertionError: 188191.9029567669 != 226839.72426476143 within 7 places (38647.82130799454 difference)

aleeciu · 2024-12-13T12:11:45Z

I opened a new PR #153

aleeciu added 3 commits February 6, 2024 10:32

add sectorial impact functions

b155af7

adjust tests due to different roundin, kwargs and function names

098033d

update tutorial

a1dac9b

aleeciu requested review from zeliest and peanutfun February 6, 2024 09:39

chahank reviewed Apr 8, 2024

View reviewed changes

bguillod mentioned this pull request Apr 29, 2024

Feature/river flooding aqueduct #108

Open

aleeciu added 3 commits August 7, 2024 14:07

Merge branch 'develop' into feature/add-missing-JRC-flood-impact-func…

bc9e612

…tions

capture unknown sector

e13cd5b

add tests looping through params and testing unknown ones

a93c9fe

chahank requested changes Aug 16, 2024

View reviewed changes

emanuel-schmid added 3 commits September 27, 2024 15:53

Merge branch 'develop' into feature/add-missing-JRC-flood-impact-func…

49221b7

…tions

Merge branch 'develop' into feature/add-missing-JRC-flood-impact-func…

98d05f1

…tions

impact_funcs.river_flood: use dicts instead of if/else blocks

ac4eec4

aleeciu added 3 commits October 29, 2024 17:03

remove unnecessary mdr attribute

7660e94

remove un-used dictionary

8e9b629

adjust object initiation

37bb860

Merge branch 'develop' into feature/add-missing-JRC-flood-impact-func…

02d02cf

…tions

emanuel-schmid merged commit 2a6837e into develop Oct 31, 2024
9 of 11 checks passed

emanuel-schmid deleted the feature/add-missing-JRC-flood-impact-functions branch October 31, 2024 10:14

emanuel-schmid restored the feature/add-missing-JRC-flood-impact-functions branch November 1, 2024 15:46

aleeciu mentioned this pull request Dec 13, 2024

Feature/add missing jrc flood impact functions #153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add missing jrc flood impact functions #111

Feature/add missing jrc flood impact functions #111

aleeciu commented Feb 6, 2024 •

edited

Loading

chahank Apr 8, 2024

aleeciu Apr 9, 2024

chahank Aug 16, 2024

chahank commented Apr 29, 2024

aleeciu commented May 3, 2024 •

edited

Loading

aleeciu commented Aug 7, 2024

chahank left a comment

chahank Aug 16, 2024

chahank Aug 16, 2024

aleeciu Oct 29, 2024

chahank Aug 16, 2024

chahank Aug 16, 2024

aleeciu Oct 29, 2024

chahank Aug 16, 2024

aleeciu commented Sep 16, 2024

emanuel-schmid commented Sep 27, 2024

aleeciu commented Oct 29, 2024 •

edited

Loading

emanuel-schmid commented Oct 31, 2024

emanuel-schmid commented Nov 1, 2024

aleeciu commented Dec 13, 2024

		@@ -35,116 +35,114 @@ def test_flood_imp_func_set(self):

		def test_region_Africa(self):

Feature/add missing jrc flood impact functions #111

Feature/add missing jrc flood impact functions #111

Conversation

aleeciu commented Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahank commented Apr 29, 2024

aleeciu commented May 3, 2024 • edited Loading

aleeciu commented Aug 7, 2024

chahank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleeciu commented Sep 16, 2024

emanuel-schmid commented Sep 27, 2024

aleeciu commented Oct 29, 2024 • edited Loading

emanuel-schmid commented Oct 31, 2024

emanuel-schmid commented Nov 1, 2024

aleeciu commented Dec 13, 2024

aleeciu commented Feb 6, 2024 •

edited

Loading

aleeciu commented May 3, 2024 •

edited

Loading

aleeciu commented Oct 29, 2024 •

edited

Loading