Skip to content
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

feat: target_market_share gains extended time-horizon for target_* outputs, to entire time-horizon of input scenario #481

Merged
merged 28 commits into from
Mar 25, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Mar 15, 2024

Note: This required some significant changes to every exported function, e.g.:
join_abcd_scenario
summarize_weighted_production
target_market_share

A quick explanation of the changes that were made:
join_abcd_scenario - now outputs a dataset that:

  • has a minimum year that is the shared minimum value of year between the abcd and scenario inputs
  • has a maximum year that is the maximum year of scenario NOTE: this means that it fills in the production and emission_factor values as NAs, since it cannot infer them from the `abcd)

summarize_weighted_production - no longer complains if it receives an input with NA values in production
target_market_share had to have some things move around a bit, as the input data structure is now slightly expanded to include this new NA values of production.

Closes #157

@jdhoffa jdhoffa requested a review from jacobvjk March 15, 2024 11:50
@jdhoffa jdhoffa added the feature a feature request or enhancement label Mar 15, 2024
@jdhoffa
Copy link
Member Author

jdhoffa commented Mar 18, 2024

Going to mark this as draft again while I look into these failings checks

@jdhoffa jdhoffa marked this pull request as draft March 18, 2024 08:55
@jdhoffa
Copy link
Member Author

jdhoffa commented Mar 18, 2024

There is an error in thepkgdown.yml as it renders the vignettes.
In particular, there is an issue now with the summarize_weighted_percent_change() function.

This issues is caused by:
A scenario input file with a start year of 2020
An abcd input file with companies that have assets that "turn on" later than 2020

the join_abcd_scenario function seems to back-fill these assets with 0 production.
The issue with this is that it "assumes" that the earlier production is 0, and thus the percent change calculation cannot take place (since a percent change from 0 is infinity).

I don't really know what to do in this case... @jacobvjk any thoughts?
Frankly my real preference here would be to begin he deprecation of BOTH summarize_weighted_percent_change AND join_abcd_scenario as I really don't know if anyone really uses these functions, but that's a bigger decision

@jdhoffa jdhoffa marked this pull request as ready for review March 18, 2024 13:47
Copy link
Member

@jacobvjk jacobvjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first set of comments. I will have to go through this again, especially the changes in join_abcd_scenario()

tests/testthat/test-join_abcd_scenario.R Outdated Show resolved Hide resolved
R/summarize_weighted_production.R Outdated Show resolved Hide resolved
R/target_market_share.R Show resolved Hide resolved
@jacobvjk
Copy link
Member

There is an error in thepkgdown.yml as it renders the vignettes. In particular, there is an issue now with the summarize_weighted_percent_change() function.

This issues is caused by: A scenario input file with a start year of 2020 An abcd input file with companies that have assets that "turn on" later than 2020

the join_abcd_scenario function seems to back-fill these assets with 0 production. The issue with this is that it "assumes" that the earlier production is 0, and thus the percent change calculation cannot take place (since a percent change from 0 is infinity).

I don't really know what to do in this case... @jacobvjk any thoughts? Frankly my real preference here would be to begin he deprecation of BOTH summarize_weighted_percent_change AND join_abcd_scenario as I really don't know if anyone really uses these functions, but that's a bigger decision

Would the deprecation of these functions remove the methodological issue or would it pop up again elsewhere?
If I imagine a company that has 0 production in t=0, I think the only valid target for that company for t=5 is still 0. That means that any such buildout is automatically extremely aligned for low carbon technologies and extremely misaligned for high carbon ones. But the analysis starts in t=0 not in t=3. So it would be counterintuitive to start setting targets for some companies in t=3 because they do not operate now. On the portfolio level, adding production in t=3 where there was none before is simply increased capacity for the overall portfolio. you do not suddenly have to build out even more or less because an asset is switched on later. the market share from t=0 is supposed to remain constant, not that of t=3.

But this is an initial intuition. Please correct me if there are flaws in my thinking here

@jdhoffa jdhoffa requested a review from jacobvjk March 20, 2024 16:17
@jdhoffa
Copy link
Member Author

jdhoffa commented Mar 20, 2024

There is an error in thepkgdown.yml as it renders the vignettes. In particular, there is an issue now with the summarize_weighted_percent_change() function.
This issues is caused by: A scenario input file with a start year of 2020 An abcd input file with companies that have assets that "turn on" later than 2020
the join_abcd_scenario function seems to back-fill these assets with 0 production. The issue with this is that it "assumes" that the earlier production is 0, and thus the percent change calculation cannot take place (since a percent change from 0 is infinity).
I don't really know what to do in this case... @jacobvjk any thoughts? Frankly my real preference here would be to begin he deprecation of BOTH summarize_weighted_percent_change AND join_abcd_scenario as I really don't know if anyone really uses these functions, but that's a bigger decision

Would the deprecation of these functions remove the methodological issue or would it pop up again elsewhere? If I imagine a company that has 0 production in t=0, I think the only valid target for that company for t=5 is still 0. That means that any such buildout is automatically extremely aligned for low carbon technologies and extremely misaligned for high carbon ones. But the analysis starts in t=0 not in t=3. So it would be counterintuitive to start setting targets for some companies in t=3 because they do not operate now. On the portfolio level, adding production in t=3 where there was none before is simply increased capacity for the overall portfolio. you do not suddenly have to build out even more or less because an asset is switched on later. the market share from t=0 is supposed to remain constant, not that of t=3.

But this is an initial intuition. Please correct me if there are flaws in my thinking here

Documenting that we discussed this in an in-person call and opened #482 as a result

jacobvjk
jacobvjk previously approved these changes Mar 25, 2024
Copy link
Member

@jacobvjk jacobvjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: bit unsure if it's better to remove the check_crucial_names for more variables than "production" or of it would be preferable to just add a switch for that. For users it won't make a difference. But I can see the point in not making the code base more complicated than necessary too.

tests/testthat/test-join_abcd_scenario.R Outdated Show resolved Hide resolved
R/summarize_weighted_production.R Outdated Show resolved Hide resolved
@jdhoffa jdhoffa requested a review from jacobvjk March 25, 2024 16:04
Copy link
Member

@jacobvjk jacobvjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jdhoffa jdhoffa merged commit ecda850 into main Mar 25, 2024
24 checks passed
@jdhoffa jdhoffa deleted the 157-extend_market_share_timeline branch March 25, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for an extended timeline for target_market_share() targets
2 participants