Closed maurolepore closed 4 months ago
Hi @maurolepore, I reviewed the reprex and have two comments:
risk_category
NA
for each grouped_by
. This may be misleading, as we don't have any benchmark for the unmatched products, but for now at least for our usage this should be OK.
_Question to you: Is it correct, that in the end on company-level the value
for risk_category
NA
in grouped_by
all
tells us how many results are unmatched? "all" is a benchmark every matched product has (it essentially compares co2 footprint to all other matched products). I.e., the NA share for that benchmark indicates the share of unmatched products._ grouped_by
NA
, but on company-level there is no grouped_by
NA
. I.e., you introduce a new benchmark on product-level but not on company-level. But then this NA
on company-level probably already counts as the "7th benchmark" as you outlined here and is therefore a high effort task: https://github.com/2DegreesInvesting/tiltIndicator/pull/639#issuecomment-1953045737
This changes the general structure of the output. Before each level of grouped_by mapped to the same number of levels of risk_category -- originally 3 ("high", "medium", and "low") and now 4 ("high", "medium", "low", NA). Now in the new output 6 of the 7 levels of grouped_by map to 4 levels of risk_category ("high", "medium", "low", NA) whereas a 7th level of grouped_by ("not_matched") maps to only one level of risk_category (NA).
tt04. Hence, I suggest we stick to the solution (https://gist.github.com/maurolepore/657d59fff496889e99cddf724d673a92) for now, if you can confirm my presumption articulated in the question under bullet "OK". And then we can discuss whether / how to fix the inconsistency outlined in "BAD" with @AnneSchoenauer next week.
Thanks for your efforts to make me understand the problems! Let me know what you think!
tt02. On company-level the
value
{where}risk_category
{is}NA
{and}grouped_by
is "all" tells us how many results are unmatched? ... I.e., theNA
share for {"all"} indicates the share of unmatched products?
Correct. I agree with your interpretation.
tt04. I suggest we stick to the solution (https://gist.github.com/maurolepore/657d59fff496889e99cddf724d673a92) for now, if you can confirm my presumption articulated in the question under bullet "OK" {tt02}.
Great. Soon I'll merge what we currently have. Then open new issues to work on what's missing.
Merges into #639
This PR adds into the same
value
theNA
s due to missing benchmarks and theNA
s due to unmatched products. This way we account for both types ofNA
s with the same 6-benchmarks we already have.reprex: https://bit.ly/tiltIndicator-729
@Tilmon I requested your review to touch base with you on my progress. But I'm still now ready to merge because I want to add more tests and refactoring.
--
TODO (myself)
*upstream()
sector*()
here or separately.TODO
EXCEPTIONS