get_region_per_countries() #1034

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

@@ -168,6 +168,46 @@ def test_get_countries_per_region(self):
        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

@@ -34,6 +35,135 @@
 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

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

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.

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?

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get_region_per_countries() #1034

get_region_per_countries() #1034

NicolasColombi commented Mar 26, 2025 •

edited

Loading

chahank left a comment

chahank Mar 26, 2025

NicolasColombi Mar 26, 2025

chahank Apr 1, 2025

chahank Mar 26, 2025

NicolasColombi Mar 26, 2025

chahank Mar 27, 2025

NicolasColombi Mar 27, 2025

NicolasColombi commented Mar 26, 2025

NicolasColombi commented Apr 1, 2025

chahank Apr 1, 2025

chahank Apr 1, 2025

chahank Apr 1, 2025 •

edited

Loading

chahank Apr 1, 2025 •

edited

Loading

chahank Apr 1, 2025

peanutfun left a comment

peanutfun Apr 9, 2025

peanutfun Apr 9, 2025

peanutfun Apr 9, 2025

peanutfun Apr 9, 2025

peanutfun Apr 9, 2025

		@@ -34,6 +35,135 @@
		LOGGER = logging.getLogger(__name__)


		class CountryCode(Enum):


		return region_name[region], impf_id[region], iso3n[region], iso3a[region]
		if code_type not in {"ISO3A", "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.")

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

get_region_per_countries() #1034

Are you sure you want to change the base?

get_region_per_countries() #1034

Conversation

NicolasColombi commented Mar 26, 2025 • edited Loading

PR Author Checklist

PR Reviewer Checklist

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

NicolasColombi commented Mar 26, 2025

NicolasColombi commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chahank Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

chahank Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peanutfun 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

NicolasColombi commented Mar 26, 2025 •

edited

Loading

chahank Apr 1, 2025 •

edited

Loading

chahank Apr 1, 2025 •

edited

Loading