Require definition of hazard type for all impact functions

[leonie-villiger]

In pull request https://github.com/CLIMADA-project/climada_python/pull/675we added haz_typeas a requirement for impact functions generated through from_sigmoid_impfor from_step_imp. In the initialization of the ImpactFunc, however, haz_type is optional. We should make this consistent.

See also the following discussion #675 (comment)

[peanutfun]

Out of curiosity: Why do we want to require this? Hazards also may have an empty ("") hazard type. If the haz_type of the impact function is set to the same value (which is the default), everything works fine. Why add this restriction?

[leonie-villiger]

@chahank: Why did we introduce the requirement for the cases from_sigmoid_impf or from_step_imp in #675?

[chahank]

The reason is that in practice Hazards always have a type. Now it is rather confusing for users in so far that they in practice must define the correct hazard type for each impact function, even though the code does not require it. In practice, this often leads to cryptic error messages in the impact or cost benefit computations. Rather than introducing many checks in said computations, I find it better to make hay_type obligatory.

Also, note that it is not a restriction as you can always choose it to be "".

Furthermore, note that in #675 did 2 things: a) allow the user to define a hazard type for from_sigmoid_impf and from_step_imp b) make this variable non-optional.

[peanutfun]

I see that a) makes a lot of sense, and since you introduced b), I also understand that for consistency, the haz_type parameter should be non-optional everywhere. But I still don't see why this should be enforced in general. Again, a default setting haz_type="" seems justified to me, because it is also the default for the Hazard base class.

The main issue about error messages is that ImpFuncSet returns an empty list if the hazard types are inconsistent. But that could be easily captured with a meaningful exception. I ran into this issue a couple of times over the last few days and the current message is very spurious: It's AttributeError: 'list' has no attribute 'calc_mdr', which is thrown here:

climada_python/climada/hazard/base.py

Line 2523 in e26f900

if impf.calc_mdr(0) == 0:

[chahank]

Good points. For the error message we already opened the issue #669 and @leonie-villiger is working on a solution.

[leonie-villiger]

Overall, I get from your comments that we're statisfied if the error messages are improved and don`t need to make haz_type mandatory for all impact functions. In that case I will close the issue again (please aprove with a thumbs up)?!

[chahank]

I would keep the issue open to remind us of the discussion as this will likely pop up again in the near future.

[peanutfun]

I don't think this needs to be kept open as a reminder anymore