-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds add_thresholds_transition_risk()
function which adds low and high thresholds for transition risk score
#790
Conversation
Hi @kalashsinghal , thanks for this! Looks good to me :) Re Dataset name:
How about calling the dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kalashsinghal
If tiltIndicator exports two internal functions (for now with # @keywords intarnal
) then we can develop this feature in isolation in tiltTransitionRisk. See my comment below including a sketch of the main components of our architecture
So I suggest these next steps:
- Move this PR to tiltTransitionRisk linking to this PR.
- Close this PR.
- Create a new PR in tiltIndicator exporting the two
*compute_profile_ranking()
functions that you need.
add_thresholds_transition_risk()
function which adds low and high thresholds for transition risk score
@maurolepore The low and high thresholds are new calculated columns for transition risk scores. How you imagine I can pass them as arguments to quantile()? The quantile() only receives |
#' Calulate `transition_risk_score` and `benchmark_tr_score` columns | ||
#' | ||
#' @param data Dataframe. | ||
#' @param profile_ranking Dataframe column. | ||
#' @param reduction_targets Dataframe column. | ||
#' @keywords internal | ||
#' @export | ||
create_tr_benchmarks_tr_score <- function(data, profile_ranking, reduction_targets) { | ||
mutate( | ||
data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maurolepore From our discussion here, I will replace this function in tiltIndicatorAfter. Should I also copy all the tests related to this function in tiltIndicator? or the tests should remain in only tiltIndicator (one package)?
/style |
Thanks @kalashsinghal, I had a first look and I need to think how this new indicator fits with the existing indicators. I already have a few ideas but I need to think more before I share them. You may install this PR in your own system and use that to draft your PR in tiltIndicatorAfter. That way you don't have to wait for me. |
I moved this PR to 2DegreesInvesting/tiltTransitionRisk#10 It'll be easier to experiment there. Since that's a new, developer-oriented package, we can merge code before we're sure of the final interfaces. Let's continue the conversation there. |
closes 2DegreesInvesting/tiltTransitionRisk#12
Dear @maurolepore @AnneSchoenauer @Tilmon
As discussed in this comment, a new dataset will output from tiltIndicator which will become input to tiltIndicatorAfter for creating the risk categories of transition risk scores (a PR will follow in tiltIndicatorAfter to show this).
This new dataset will have low and high thresholds for transition risk scores of around 13k
activity_uuid_product_uuid
s. I haven't come up with a good name yet to describe this new dataset, so please suggest any good name for better understanding! At the moment, I have named the functiontrs_low_high_thresholds_for_ecoinvent_all
which will output this new dataset.As mentioned in this comment, the function
trs_low_high_thresholds_for_ecoinvent_all
will have a new input from tiltIndicatorBefore after merging this PR (named assp_all_uuids_scenario_sectors
) and old inputsscenarios
andco2
. Theco2
dataset will be used for creating the profile ranking for all 13k uuids, and datasetssp_all_uuids_scenario_sectors
andscenarios
will be used for creating the reduction targets for all 13k uuids. The profile ranking and reduction targets will be used to create the transition risk scores. These scores will then finally creates the low and high thresholds for transition risk scores.I didn't have a toy dataset for
sp_all_uuids_scenario_sectors
, hence I used thetoy_sector_profile_companies
dataset with some modifications to create it. However, they are not the same and we can think of creating a new toy dataset forsp_all_uuids_scenario_sectors
in tiltToydata.Here is a reprex for better understanding:
Created on 2024-05-30 with reprex v2.0.2
This PR is still work in progress and I will write unittests in upcoming code. Thanks for the review!
TODO