CLIMADA-project / climada_petals

See https://github.com/CLIMADA-project/climada_python first
GNU General Public License v3.0
22 stars 13 forks source link

Further improvement of the supplychain module #124

Open spjuhel opened 2 months ago

spjuhel commented 2 months ago

The module provides an excellent basis for indirect impact computation, but there are several aspects in the code structure that could largely benefit from a rework.

  1. Most methods do too many things at once, more granularity would notably ease unit tests
  2. The methods calc_shock_to_sectors and calc_impact can quickly confuse the user. Notably, as they both accept exposure and impact as arguments, thus the entry point for exposure and impact is unclear.
  3. In particular, there is a concerning block in calc_impact where exposure and impact arguments are ignored if a shock was already computed. (note that #81 will add a warning)
  4. Multiple attributes of a SupplyChain object are initially Noneand are set by methods thereafter. This can create confusion on when attributes are correctly computed, and may lead to incoherent objects.

I propose to improve the implementation with the following (and could work on that once #81 is merged, or later, this is mostly to track ideas). I see at least two ways to do this:

  1. Keep a single SupplyChain class, and improve its initialisation and the distinction between calc_shock_to_sectors and calc_impact. The core idea would be that instantiating a SupplyChain object would by default require both MRIOT, Exposure and Impact, and create an object which holds direct impact translated into MRIOT format, from which indirect impact can then be computed.

Pros:

  1. Separate things more by creating two new classes, say IO_Shock and IndirectImpact (Other suggestions welcome!), such that, IO_Shock does the translation of a CLIMADA impact into a MRIOT formatted one, and IndirectImpact does the computation.

Pros:


05/06/24: Approach chosen is 2.

Other sub-issues that should be addressed:

Features that could be added / or should be thought of while doing the rework:

aleeciu commented 2 months ago

Features that could be added / or should be thought of while doing the rework:

Integration of the Inoperability model Integration of the MRIA model Feedback loop between indirect shocks and direct shocks for multiple event case (better account for the fact that what is destroyed cannot be destroyed again)

Thanks a lot @spjuhel - this is quite alinged with what I had in mind!

ChrisFairless commented 2 months ago

I like these ideas, and having a granular, pythonic refactor would be lovely.

ChrisFairless commented 2 months ago

I also have an additional functionality request! Maybe this should be a separate Issue. But I'll start here for the purpose of discussion since it seems at least a little relevant.

One particular bit of granularity that I'd like is flexibility with the calc_shock_to_sectors method. Currently It takes an Impact and an Exposures object as input, aggregates them by region_id, and translates that as a direct shock to the MRIOT tables based on the requested sectors. The flexibility I need is to work with larger numbers of Impacts and Exposures. Namely:

My proposed solution here is to separate calc_shock_to_sectors functionality into two bits of functionality: namely calculating shocks and combining shocks, letting you aggregate across multiple Impacts/Exposures/sectors. I don't know the best way to implement them because the order can be flexible: aggregating can be done before or after the main part of calc_shock_to_sectors. That is,

Both of these solutions allow me to work with large numbers of Impact objects and Exposures more elegantly. To deal with the impact matrix question, I would also add a check in calc_shock_to_sectors that spots when an Impact is only for a single country/region_id and then uses the Impact.at_event property instead of the marginal sums of the Impact.imp_mat, since these are equivalent in this case.

For now I have a local workaround with the second method. I'd be happy to turn it into a pull request!