Add impact classmethod to concatenate several impact objects #529

raphael-portmann · 2022-08-08T07:40:12Z

UPDATES

now includes tutorial example
Now includes test
checks are defined as separate functions
ISSUE: due to the many checks, the code is rather complex

Changes proposed in this PR:

add a classmethod with two concatenation options to concatenate two or more impact objects with the same hazard type, frequency, and coordinate system that either (1) have the same exposure but contain events with different dates / time periods of the year (e.g. seasonal data) or (2) have the same event dates but a different exposure (e.g. two different countries)

To Do list

implement exposure concatenation option
make tests

Open questions

Is option (2) really needed or would one just merge the exposure before the impact calculation?
Is it a problem that, after concatenation, the data is not sorted by date (e.g. in the impact matrix and event ids)?

Pull Request Checklist

Correct target branch selected (if unsure, select develop)
Source branch up-to-date with target branch
Docs updated
Tests updated
Tests passing

chahank

Thanks @raphael-portmann ! This will come in very handy :D

For the moment, the code is still a bit raw. Please update the cosmetics (white spaces, lines, etc.) to make the code cleaner.

In addition, I suggest that you group all the checks into individual functions that can then be called. This will make the code much more readable and modular.

I also suggest that you now write test methods. Try also to do some testing with 'real data' from colleagues to find the hidden issues quickly.

climada/engine/impact.py

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

to continue working on feature with up to date version

chahank · 2022-12-09T10:03:04Z

climada/engine/impact.py

+        Parameters
+        ----------
+        imp_list : list of climada.engine.Impact objects
+                list of Impact object to concatenate


I think your spacing for the docstrings are off.

chahank · 2022-12-09T10:04:04Z

climada/engine/impact.py

+                raise ValueError(
+                    "Not all impacts have a hazard type."
+                    "Please specify the same hazard type for each impact."
+                    "The impacts are incompatible and cannot be concatenated.")


I do not really like the sentence you added to all errors The impacts are incompatible and cannot be concatenated. What is that supposed to mean?

I second this. The error traceback shows that it occurs within the concat method, so there is no need to mention that it did not work as expected 😉

chahank · 2022-12-09T10:05:16Z

climada/engine/impact.py

+        tot_value = imp_list[0].tot_value
+
+        #fill remaining attributes
+        tag = imp_list[0].tag


I still think it is not ok to discard all tags other than for the first instance of impact.

chahank · 2022-12-09T10:06:19Z

climada/engine/test/test_impact.py

+    def test_concat_pass(self):
+        """test concatenation of impacts"""
+
+        def test_haztype_unit_crs_attrs(self,imp1,imp2,imp3,imp3_3,imp3_2):


Suggested change

def test_haztype_unit_crs_attrs(self,imp1,imp2,imp3,imp3_3,imp3_2):

def test_haztype_unit_crs_attrs(self, imp1, imp2, imp3, imp3_3, imp3_2):

climada/engine/test/test_impact.py

peanutfun · 2023-01-19T16:15:07Z

climada/engine/impact.py

+                raise ValueError(
+                    "Not all impacts have a hazard type."
+                    "Please specify the same hazard type for each impact."
+                    "The impacts are incompatible and cannot be concatenated.")


I second this. The error traceback shows that it occurs within the concat method, so there is no need to mention that it did not work as expected 😉

climada/engine/impact.py

climada/engine/test/test_impact.py

# Conflicts: # climada/engine/impact.py

peanutfun

Almost there! Please use "handmade" test data and check that concetation works based on the resulting values and not just the lengths of the arrays. Also, the docstring could explain a little bit better what's happing internally. Please make sure that the docs can be built and check the appearance of the docstring in the preview

climada/engine/test/test_impact.py

climada/engine/impact.py

doc/tutorial/climada_engine_Impact.ipynb

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>

* Simplify concatenation of Impact attributes. * Add 'frequency_unit' attribute to concatenation. * Fix docstring formatting. * Update tests to use fixtures and concat empty impact matrices. * Format code using black.

peanutfun · 2023-01-27T15:27:02Z

I reworked this a bit, tidying up the code and the docstring and improving the tests. Now even the linter is happy! Merging...

peanutfun · 2023-01-27T15:28:51Z

@raphael-portmann Thanks for your contribution! 🥳

Raphael Portmann added 5 commits July 26, 2022 15:18

added classfunction Impact.concat

48d46cd

Merge branch 'develop' into feature/impact-merge

97dfd64

update description of impact.concat

28973d5

handle some jenkins warnings on indentation and line length

2768c11

update method description

a619dba

raphael-portmann requested a review from chahank August 8, 2022 07:43

chahank requested changes Aug 8, 2022

View reviewed changes

raphael-portmann and others added 16 commits August 8, 2022 15:04

Update climada/engine/impact.py

27b54d4

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

09a2bfd

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

661d86f

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

acf2e95

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

ae80ca9

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

1ad8e7b

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

cbc6edc

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

5178add

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

b337ace

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

45a861f

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

80f2bb5

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

f48bb03

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

3ddbbdb

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

Update climada/engine/impact.py

281db05

Co-authored-by: Chahan M. Kropf <chahan.kropf@usys.ethz.ch>

initialize new impact directly with new attributes

86cd4df

some cosmetics

2e53164

emanuel-schmid marked this pull request as draft August 10, 2022 11:30

Raphael Portmann added 6 commits October 10, 2022 13:48

resolved conflict with develop by integrating both changes

f6b937e

make functions out of checks, some cosmetics

aa50c26

Merge branch 'develop' into feature/impact-merge

b5bc157

some minor cosmetics

ff351ed

start writing test for impact.concat

8294c64

Merge branch 'develop' into feature/impact-merge

d9f56ba

to continue working on feature with up to date version

Raphael Portmann added 2 commits December 8, 2022 11:56

group tests into submethods

5f91332

add example of impact.concat to tutorial

9ca57e3

raphael-portmann requested a review from chahank December 8, 2022 16:15

chahank reviewed Dec 9, 2022

View reviewed changes

climada/engine/test/test_impact.py Outdated Show resolved Hide resolved

peanutfun requested changes Jan 19, 2023

View reviewed changes

Raphael Portmann added 5 commits January 24, 2023 16:09

strongly simplified impact.concat

61bd7f1

impact.concat simplified and working

1fdf377

updated tests

42ab31c

Merge branch 'develop' into feature/impact-merge

3311ccd

# Conflicts: # climada/engine/impact.py

some cosmetics

640a8ac

raphael-portmann requested a review from peanutfun January 25, 2023 14:14

undo change in climada.hazard.base

1328cdd

peanutfun requested changes Jan 25, 2023

View reviewed changes

Apply suggestions from code review

63c64c5

Co-authored-by: Lukas Riedel <34276446+peanutfun@users.noreply.github.com>

peanutfun mentioned this pull request Jan 26, 2023

LitPop exposures used in Impact tutorial changed considerably since October 2021 #630

Closed

Raphael Portmann added 2 commits January 26, 2023 11:03

tests redone, small change in impact.concat

62b51f1

re-run notebook to fix issue with it

aec319c

raphael-portmann requested a review from peanutfun January 26, 2023 12:56

peanutfun added 2 commits January 27, 2023 15:41

Finalize Impact.concat

8bd9d74

* Simplify concatenation of Impact attributes. * Add 'frequency_unit' attribute to concatenation. * Fix docstring formatting. * Update tests to use fixtures and concat empty impact matrices. * Format code using black.

Reduce local variable count in Impact.concat

0d75be2

peanutfun approved these changes Jan 27, 2023

View reviewed changes

peanutfun marked this pull request as ready for review January 27, 2023 14:53

peanutfun merged commit d3f585b into develop Jan 27, 2023

emanuel-schmid deleted the feature/impact-merge branch January 31, 2023 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add impact classmethod to concatenate several impact objects #529

Add impact classmethod to concatenate several impact objects #529

raphael-portmann commented Aug 8, 2022 •

edited

Loading

chahank left a comment

chahank Dec 9, 2022

chahank Dec 9, 2022

peanutfun Jan 19, 2023

chahank Dec 9, 2022

chahank Dec 9, 2022

peanutfun Jan 19, 2023

peanutfun left a comment

peanutfun commented Jan 27, 2023

peanutfun commented Jan 27, 2023

	def test_haztype_unit_crs_attrs(self,imp1,imp2,imp3,imp3_3,imp3_2):
	def test_haztype_unit_crs_attrs(self, imp1, imp2, imp3, imp3_3, imp3_2):

Add impact classmethod to concatenate several impact objects #529

Add impact classmethod to concatenate several impact objects #529

Conversation

raphael-portmann commented Aug 8, 2022 • edited Loading

To Do list

Open questions

Pull Request Checklist

chahank left a comment

Choose a reason for hiding this comment

chahank Dec 9, 2022

Choose a reason for hiding this comment

chahank Dec 9, 2022

Choose a reason for hiding this comment

peanutfun Jan 19, 2023

Choose a reason for hiding this comment

chahank Dec 9, 2022

Choose a reason for hiding this comment

chahank Dec 9, 2022

Choose a reason for hiding this comment

peanutfun Jan 19, 2023

Choose a reason for hiding this comment

peanutfun left a comment

Choose a reason for hiding this comment

peanutfun commented Jan 27, 2023

peanutfun commented Jan 27, 2023

raphael-portmann commented Aug 8, 2022 •

edited

Loading