search

brightway-lca / brightway2-data

Tools for the management of inventory databases and impact assessment methods. Part of the Brightway LCA framework.
https://docs.brightway.dev/
BSD 3-Clause "New" or "Revised" License
11 stars 24 forks source link

add include_substitution option to technosphere() #173

Closed TimoDiepers closed 5 months ago

TimoDiepers commented 5 months ago

This adds the option to include substitution exchanges in Activity.technosphere(). There is a similar option already for Activity.production(), but if one wants to (e.g. temporally) remap activities, it can be useful to treat substitution exchanges like technosphere exchanges.

We came across this when investigating https://github.com/brightway-lca/bw_timex/issues/53 and found this a possibly useful addition in general

codecov[bot] commented 5 months ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

tngTUDOR commented 5 months ago

@TimoDiepers , it would also be great if you could add a test to verify that this works as you expect it.

TimoDiepers commented 5 months ago

@tngTUDOR Sure, adding a test is no problem. What might be a problem though is that I just saw that @cmutel actually removed this exact option of include_substitution in v4.0.DEV39 and it sounds like there was a strong opinion on this: https://github.com/brightway-lca/brightway2-data/blob/b0265f2cb53df557888ba267f8ee0f0a4747749f/CHANGES.md?plain=1#L20 I agree with the argument that, logically, a substitution is not an input. However, I'm not sure if that means that it shouldn't be possible to treat it like one.

Example: One creates a copy of a process and wants to keep all exchanges (or at least their amounts and types), but relink them to another database for the "input" processes. If I want to relink all exchanges to another database,I'd also want to relink substitution exchanges (but not the actual "self-production exchange"). If one could include substitution exchanges in Node.technosphere(), one could just iterate over all of those exchanges and relink them. Otherwise, one would have to iterate over all technosphere and production exchanges and filter out the "self-production exchanges" which should not be relinked (which could also cause problems when considering multifunctional topics I guess? But that's probably a different story).

Anyway, I'd be happy to hear your thoughts on this - of course I'd understand if you think this generally shouldn't be an option in the code.

TimoDiepers commented 5 months ago

Closing this PR - we now just iterate over both technosphere() and substitution() so it's not that big of a problem after all, and this keeps the code "clean", avoiding potential misinterpretation of substitution exchanges ;)