WIP: Feature/set imp matrix

[chahank]

This is a first step in an attempt to attack the problem of methods that modify CLIMADA objects in place which can lead to inconsistencies (such as e.g. discussed for the meta parameter in #348 ). This would also be a step towards better __init__ methods as discussed e.g. in #342.

Here we add methods that compute the impact attributes at_event, eai_exp and aai_agg directly from the imp_mat and the frequency (no testing).

These methods are so far not embedded in the rest of the method from the class impact, but can in a future iteration if this pull request is deemed useful. Furthermore, these methods come in handy for instance in the handling of line and polygons exposures c.f. https://github.com/CLIMADA-project/climada_python/tree/feature/lines_polygons_exp or multi impact yearsets c.f. https://github.com/CLIMADA-project/climada_python/tree/feature/sets .

Feedback on the proposed methods would be appreciated.

[Evelyn-M]

Small detail, the referenced issue above, @chahank, does not match the __init__ methods discussion you refer to. Can you find the right hashtag?

[Evelyn-M]

So currently the at_event is calculated from an impact variable (line 1033) which considers potential deductibles, covers etc. The imp_mat does not previously account for those, right? Aka the at_event would have to be modified afterwards, if such conditions exist?

[Evelyn-M]

(similary for eai_exp, etc.)

[chahank]

Yes, this is then to be added in this method, good point! My issue with the current implementation is that the at_event and eai_exp are computed in a very obscure manner inside of the .calc method.

[chahank]

Best would probably be to have a class method calc_impact and calc_residual_impact (if with insurance stuff).

[chahank]

If I am not mistaken, the deductible all act on the impact matrix as implemented right now? Thus, the imp_mat is actually not the risk matrix, but the residual risk matrix after insurance deductibles have been applied.

My suggestion would thus be to disentangle that as it is unclear at the moment.

[Evelyn-M]

I find this way of defining the attributes very neat and clear, in general.

[ChrisFairless]

I missed some of these conversations: what risk are we mitigating here? Are there parts of an Impact object that get out of sync? That is, is there a risk that Impact.imp_mat is overwritten after eai_exp, at_event or aai_agg are set?

If that's the case we should consider other options:

• Don't let that happen: any method that would modify imp_mat returns a fresh Impact object
• Make properties eai_exp, at_event or aai_agg methods as well, meaning they're recalculated whenever they're accessed

Skimming the code for the class, I think the only method that breaks this rule is Impact._exp_impact, which is hidden and only used in Impact.calc anyway, so the risk is low. Has it become an issue elsewhere?

[chahank]

I missed some of these conversations: what risk are we mitigating here? Are there parts of an Impact object that get out of sync? That is, is there a risk that Impact.imp_mat is overwritten after eai_exp, at_event or aai_agg are set?

Currently, the Impact objects are initialized with an empty constructors, which is then filled in a intransparent manner by the method .calc. This is sub-optimal, an prevents the use of Impact objects in other parts of the code, such as the lines_polygons_handler or the yearsets.

[chahank]

If that's the case we should consider other options:

Don't let that happen: any method that would modify imp_mat returns a fresh Impact object
Make properties eai_exp, at_event or aai_agg methods as well, meaning they're recalculated whenever they're accessed

Yes and no. The problem is not with the modification of the imp_mat only, but with the whole class. Instead of trying to apply a restriction on future methods handling Impact, it would be cleaner to make the __init__ or other initialization methods more consistent.

Making the eai_exp, aai_agg or at_event properties has two main disadvantages: 1) it uses unecessary computation time 2) the current Impact API default is to NOT save the imp_mat value. This is because the imp_mat matrix can become very large, and often only the reduced arrays are needed. This should probably not be changed. Therefore, instead of properties, I propose here separate methods.

[chahank]

Thanks @ChrisFairless and @Evelyn-M for the great comments! This is still a very early stage of development, so all your input is very much appreciated.

[ChrisFairless]

It's nice these are all static methods as well - it means if you're doing something really janky with impacts you can do it outside of the class

[ChrisFairless]

Thanks for the explanation @chahank.

These make sense then. Is the idea that Impact.__init__ would basically be Impact.calc?

[chahank]

These make sense then. Is the idea that Impact.init would basically be Impact.calc?

Not sure yet. Probably not. I would like a generic __init__ which can then be used in Impact.calc which would become a class method probably.

[chahank]

Thanks for the discussion! The main points were included in a larger refactoring of the Impact class. The work is currently on the branch feature/refactor_impact_calc. Please feel free to come to help with the refactoring efforts.

[chahank]

This pull request was addressed in PR #436