get_region_per_countries() by NicolasColombi · Pull Request #1034 · CLIMADA-project/climada_python

NicolasColombi · 2025-03-26T16:20:17Z

Changes proposed in this PR:

Convert only internally available variables into enum class
Access such variables from a new function get_region_per_countries() to assign impact function ids

PR Author Checklist

PR Reviewer Checklist

…tries

chahank

Very useful to simplify the use of the tropical cyclone impact function.

There are now many tests failing, can you please fix that?
Please update the changelog

chahank · 2025-03-26T16:50:31Z

climada/entity/impact_funcs/trop_cyclone.py

+        )
+
+    @staticmethod
+    def get_regions_per_countries(


Can you please add tests for this method?

Would the name get_impf_id_regions_per_countries be clearer?

Just give it a thought: would you have found the use of this function with the current name if you had not written it just by looking at the class?

chahank · 2025-03-26T16:52:05Z

climada/entity/impact_funcs/trop_cyclone.py

+            key
+            for country in countries
+            for key, value in country_dict.items()
+            if country in value


Are you sure you want to check inclusion and not equality?

I think so, as value is the list of countries iso codes corresponding to the region ID key. Consequently, I want to check if the country iso code country is included in that list.

Ok, then can use a better name than value? For me, value means a single value, not a list. The same for key which is unclear what this is.

Yes sure, normally key & value refers to the key-value pair in a dictionary, but I guess the meaning of key and value is clear only if you know what the dictionary is... it should be clearer now.

NicolasColombi · 2025-03-26T21:01:54Z

Very useful to simplify the use of the tropical cyclone impact function.

There are now many tests failing, can you please fix that?

Please update the changelog

Done ✅

NicolasColombi · 2025-04-01T06:51:52Z

I think this is ready, what do you think @chahank ?

chahank · 2025-04-01T11:44:18Z

climada/entity/impact_funcs/test/test_tc.py

+    def test_get_region_per_countries(self):
+        """Test static get_regions_per_countries()"""
+        ifs = ImpfSetTropCyclone()
+        out = ifs.get_regions_per_countries(countries=["CHE"], code_type="ISO3A")


Can you please use more clear variable names. out is not very clear here. Can you also make clear why the outputs are what they are?

chahank · 2025-04-01T11:46:40Z

climada/entity/impact_funcs/trop_cyclone.py

+            a string if the code is iso3a or an integer if ISO3N. For example, for Switzerland:
+            the ISO3A code is "CHE" and the ISO3N is 756.
+        code_type : str
+            Either "ISO3A" or "ISO3N".


Please write clearly what this is. ISOA and ISO3N are not official acronyms. Please correct in all the code, the error messages, and the tests.

chahank · 2025-04-01T11:47:42Z

climada/entity/impact_funcs/trop_cyclone.py

+        regions_ids : list
+            List of the regions that match the countries.
+        regions_names : list
+            List of the regions that match the countries.


These are identical docstrings, yet different variables. PLease be clear. I think you mean that one contains the ids defined in the paper by Eberenz, the other the region names?

chahank · 2025-04-01T11:48:45Z

climada/entity/impact_funcs/trop_cyclone.py

+            raise ValueError("code_type must be either 'iso3a' or 'iso3n'")
+        elif not all(isinstance(country, type(countries[0])) for country in countries):
+            raise ValueError("All elements in the list must be of the same type.")
+        elif code_type == "ISO3A" and isinstance((countries[0]), int):
+            raise ValueError("ISO3A code type cannot have integer values.")
+        elif code_type == "ISO3N" and isinstance((countries[0]), str):
+            raise ValueError("ISO3N code type cannot have string values.")


Please choose clear words / variable names. Here it is neither consistent (e.e. iso3a and ISO3A), nor standard.

chahank · 2025-04-01T11:49:26Z

climada/entity/impact_funcs/test/test_tc.py

        self.assertListEqual(out[2], [124, 840])
        self.assertListEqual(out[3], ["CAN", "USA"])

+    def test_get_region_per_countries(self):


Please add at least one test with multiple countries.

climada/entity/impact_funcs/trop_cyclone.py

Co-authored-by: Chahan M. Kropf <[email protected]>

peanutfun

Thanks for the contribution! However, I have a lot of questions design-wise. What you implemented seems unnecessarily restrictive to me: Users have to enter a list of either exclusively strings or ints, and then tell the function in a separate parameter whether they entered strings or ints 😬 Also, the enum doubles functionality with country_to_iso, which allows for converting numerical to alpha-3 ISO codes and vice versa.

Overall, the function/enum combination can be made much more accessible and concise, I think.

peanutfun · 2025-04-09T07:42:55Z

climada/entity/impact_funcs/trop_cyclone.py

 LOGGER = logging.getLogger(__name__)


+class CountryCode(Enum):


Why does this enum contain numerical and alpha-3 country codes? Conversion between the two can be done by country_to_iso and this effectively doubles the functionality

Sure, only one is enough, just thought it was better practice to have both in case one might need them, I can delate one or the other if you prefer.

Yes, it is just that the conversion should be done ideally with the existing method so that we do not duplicate existing code.

Ok, I will delate one then 👍

peanutfun · 2025-04-09T07:43:39Z

climada/entity/impact_funcs/trop_cyclone.py

+        code_type : str
+            Either "ISO3A" or "ISO3N". "ISO3A" stands for "ISO 3166-1 alpha-3" which is a
+            three-letter country code, "ISO3N" stands for "ISO 3166-1 numeric" which is a
+            three-digit country code, the numeric version of "ISO3A". For example, for Switzerland:
+            the "ISO3A" code is "CHE" and the "ISO3N" is 756.


This is inconsistent with country_to_iso, where the two types are called numeric and alpha-3

get_countries_per_region also uses iso3a and iso3n, so we need to chose which name to use consistenly in all climada code. The official would be "ISO 3166-1 alpha-3" and "ISO 3166-1 numeric".

That's unfortunate. I think numeric and alpha-3 are clearest and least error-prone. Note that there is also an alpha-2 representation for many countries, so iso3a is not precise.

Let's go for numeric and alpha-3 then. Shall I change it in the other method too ? (get_countries_per_region)

peanutfun · 2025-04-09T07:47:26Z

climada/entity/impact_funcs/trop_cyclone.py

+        """

-        return region_name[region], impf_id[region], iso3n[region], iso3a[region]
+        if code_type not in {"ISO3A", "ISO3N"}:


Why is code_type a parameter at all? If any item in the list is a str, it must be an alpha-3 code. If it is int, it must be a numerical code.

Agree, it is not really needed nor optimal 👍

peanutfun · 2025-04-09T07:47:57Z

climada/entity/impact_funcs/trop_cyclone.py

+        elif not all(isinstance(country, type(countries[0])) for country in countries):
+            raise ValueError("All elements in the list must be of the same type.")


Why? It's easy to decide on a per-element basis if a numerical or an alpha-3 code is given.

peanutfun · 2025-04-09T07:49:17Z

climada/entity/impact_funcs/trop_cyclone.py

+        impf_ids : list
+            List of impact function ids matching the countries.


Why not immediately return the appropriate impact function (set)s?

Don't you typically define an impact function id column in the exposure ? (this is what I had in mind as an application of this function)

Ok, understood. You can keep it as is then.

Ok. But this only makes sense then if nobody changes the regions and impf IDs defined in from_calibrated_regional_ImpfSet:

climada_python/climada/entity/impact_funcs/trop_cyclone.py

Line 261 in 7205ee7

def from_calibrated_regional_ImpfSet(

I think this method should then also use the information in CountryCode and not re-define it.

Also here:

climada_python/climada/entity/impact_funcs/trop_cyclone.py

Line 397 in 7205ee7

regions_short = ["NA1", "NA2", "NI", "OC", "SI", "WP1", "WP2", "WP3", "WP4"]

Agree, I did not saw the other 2 functions. Then I guess we just need to use CountryCodein these two.

peanutfun · 2025-05-28T08:13:50Z

@NicolasColombi What's the status here?

NicolasColombi · 2025-05-28T08:16:33Z

@NicolasColombi What's the status here?

I'll take some time next week to implement your suggestions, as for the other PRs as well.

chahank · 2025-08-20T13:55:54Z

@NicolasColombi it would be good to finish this PR soon for the next release in the middle of September.

NicolasColombi · 2025-08-21T09:24:57Z

Hi there @chahank @peanutfun ! I eventually incorporated your suggestions... Let me know what you think of the current version. One thing we need to decide is a consistent use of country codes acronyms: some climada functions uses ISO3A/N and others numeric or alpha-3, the official would be ISO 3166-1 alpha-3 and ISO 3166-1 numeric.

NicolasColombi · 2025-08-21T13:29:10Z

Okei, I delated the numeric codes in the Enum and used the country_to_iso to convert numeric codes to alpha-3, I also homogenize the use of numeric and alpha-3 throughout the class and modified the other functions that were defining regions so that they use the Enum class.

NicolasColombi · 2025-08-28T06:54:46Z

Okei, I delated the numeric codes in the Enum and used the country_to_iso to convert numeric codes to alpha-3, I also homogenize the use of numeric and alpha-3 throughout the class and modified the other functions that were defining regions so that they use the Enum class.

@chahank @peanutfun I will be on holidays for the next two week. If you require additional changes I will be available from Sept.15, or if the changes are really small, I might carve out some time tomorrow. I think this PR should be ready now.

chahank · 2025-08-28T09:32:03Z

@peanutfun if this is fine for you, I would let you finalize the review for this PR.

climada/entity/impact_funcs/trop_cyclone.py

peanutfun · 2025-09-03T14:52:45Z

Jenkins compatibility test hangs, but the GitHub one passed. Merging. Great work, @NicolasColombi!

create enum class for iso codes and add function get_regions_per_coun…

d2aa357

…tries

NicolasColombi requested a review from chahank March 26, 2025 16:20

chahank requested changes Mar 26, 2025

View reviewed changes

NicolasColombi added 2 commits March 26, 2025 20:19

fix pylit and update changelog

a915aba

fix failing tests

fb7d78d

add test

bd62ddc

NicolasColombi marked this pull request as ready for review March 26, 2025 22:16

change variable names in list comprehension

7fa70e5

chahank reviewed Apr 1, 2025

View reviewed changes

climada/entity/impact_funcs/trop_cyclone.py Outdated Show resolved Hide resolved

Update climada/entity/impact_funcs/trop_cyclone.py

4c08a08

Co-authored-by: Chahan M. Kropf <[email protected]>

NicolasColombi requested review from emanuel-schmid and peanutfun as code owners April 9, 2025 05:52

update strings and func name

7205ee7

peanutfun requested changes Apr 9, 2025

View reviewed changes

NicolasColombi added 2 commits August 21, 2025 11:18

delate code type arg and other

215aece

Merge branch 'develop' into feature/get-region-per-country

f6ceac0

NicolasColombi added 2 commits August 21, 2025 14:35

remove numeric from enum, convert numeric to alpha3

bb0db52

fix tests

0520131

emanuel-schmid reviewed Sep 1, 2025

View reviewed changes

climada/entity/impact_funcs/trop_cyclone.py Show resolved Hide resolved

peanutfun added 2 commits September 3, 2025 15:22

Fix linter and formatting issues

b95c1e7

Fix a bug in test

0ad6ffa

peanutfun merged commit 6cc6e08 into develop Sep 3, 2025
18 of 19 checks passed

emanuel-schmid deleted the feature/get-region-per-country branch September 4, 2025 08:37

		LOGGER = logging.getLogger(__name__)


		class CountryCode(Enum):

		elif not all(isinstance(country, type(countries[0])) for country in countries):
		raise ValueError("All elements in the list must be of the same type.")

		impf_ids : list
		List of impact function ids matching the countries.

Conversation

NicolasColombi commented Mar 26, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

NicolasColombi commented Mar 26, 2025

Uh oh!

NicolasColombi commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chahank Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chahank Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peanutfun 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!

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!

NicolasColombi Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

peanutfun commented May 28, 2025

NicolasColombi commented Mar 26, 2025 •

edited

Loading

chahank Apr 1, 2025 •

edited

Loading

chahank Apr 1, 2025 •

edited

Loading

NicolasColombi Aug 21, 2025 •

edited

Loading