CLIMADA-project / climada_python

Python (3.8+) version of CLIMADA
GNU General Public License v3.0
298 stars 118 forks source link

Applying a measure throws an error if impact function id is not of type int #925

Open mncosta opened 1 month ago

mncosta commented 1 month ago

Describe the bug When applying a measure that aims to map from one impact function to another, error is thrown if impact functions id are not of type int. Yet from the docs, id could be of type int | str.

To Reproduce Steps to reproduce the behavior/error:

  1. Have a measure with imp_fun_map set to change a impact function with a str id, i.e., "HZtoHZ1"

Why doesn't the param imp_fun_map receive a tuple(int | str, int |str), just like the other params:

This would enable having either int or str as types of impact functions id, and one would bypass the need to parse the string imp_fun_map as well?

Code example (following the first example on the docs)

%matplotlib inline
import numpy as np
from climada.entity import ImpactFuncSet, ImpfTropCyclone, Exposures
from climada.entity.measures import Measure
from climada.hazard import Hazard

# define measure
meas = Measure(
    name='Mangrove',
    haz_type='TC',
    color_rgb=np.array([1, 1, 1]),
    cost=500000000,
    imp_fun_map='HZtoHZ1',
)

# impact functions
impf_tc = ImpfTropCyclone.from_emanuel_usa()
impf_tc.id = 'HZ'
impf_all = ImpactFuncSet([impf_tc])
impf_all.plot();

# dummy Hazard and Exposures
haz = Hazard('TC') # this measure does not change hazard
exp = Exposures() # this measure does not change exposures

# new impact functions
new_exp, new_impfs, new_haz = meas.apply(exp, impf_all, haz)
axes = new_impfs.plot();
axes.set_title('TC: Modified impact function')

Expected behavior Impact function id on the new exposure objecto should be mapped to new id even if ids are strings.

Screenshots

image

Climada Version: 4.1.0

System Information (please complete the following information):

Additional context None

chahank commented 1 month ago

Thanks for reporting this. The simplest is to always use integers for impact function ids. We might update this in the future and require integers for the impact function ids.

We are also in the process of revising the measure class and will for sure fix this in the process.

peanutfun commented 1 month ago

In many cases, using a string as ID might work, but we generally assume that the IDs are integers. Therefore, I suggest we continue not enforcing a type, but update the docs of ImpactFunc such that id is expected to be int.