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

Add equity weighting option to compute scc function #40

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Sep 1, 2021

No description provided.

@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #40 (3dfd68f) into master (fd67fa9) will increase coverage by 0.32%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   89.47%   89.79%   +0.32%     
==========================================
  Files          14       14              
  Lines         285      294       +9     
==========================================
+ Hits          255      264       +9     
  Misses         30       30              
Flag Coverage Δ
unittests 89.79% <100.00%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/marginaldamage.jl 66.19% <100.00%> (+4.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd67fa9...3dfd68f. Read the comment docs.

@lrennels
Copy link
Collaborator Author

lrennels commented Sep 1, 2021

@davidanthoff and @FrankErrickson this (1) adds the option to use equity weighting for RICE SCC calculations (2) fixes some old documentation that indicated the default settings were constant discounting, while I believe they have already been changed to default to Ramsey discounting.

Copy link
Collaborator

@FrankErrickson FrankErrickson left a comment

Choose a reason for hiding this comment

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

I had a couple questions in the code. I didn't review some of the Mimi internal pieces too closely as I don't fully understand how all of it fits together for the scc function. I think the equity-weighted discount factors looked ok too, but always a good idea to have @davidanthoff just look over that equation really quickly.


year_index = findfirst(isequal(year), model_years)

if isnothing(normalization_region)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this calculation? Couldn't we just index into the already calculated cpc terms from above using the year_index for the specified normalization region?

Copy link
Collaborator Author

@lrennels lrennels Sep 13, 2021

Choose a reason for hiding this comment

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

@FrankErrickson you're right, we can probably simplify here by using the cpc from above, with something like the following.

cpc_0 = isnothing(normalization_region) ?  mean(cpc[year_index, :]) : cpc[year_index, normalization_region]

cpc_0 = 1000 * c[year_index, normalization_region] / pop[year_index, normalization_region]
end

df = zeros(year_index-1, num_regions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the correct size for df? If I added a pulse in 2010, year_index would be 6 I think? So now you would only have discount factors as a 5x12 array?

I think if you are only calculating discount factors for the periods after the pulse, you'd need something spanning from the year of pulse to the end of model time horizon. I'm not totally sure how you set up this internal SCC Mimi code. But in my own code, I would also just set discount factors to 0.0 for any periods before the pulse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the deleted line 51 below, I think it essence were were starting with an array of zeros of length year_index-1 and then concatenating that with one value calculated for each year. I think this mimics that with arrays, starting with a zeros array which would be 5x12 for a year_index of 6 and then appending a set of discount factors, one per region, for each of the following years between year and last_year inclusive. I can probably do it more elegantly but does that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for clarity the line I'm referring to is

df = [zeros(year_index-1)..., ((global_cpc[year_index]/global_cpc[i])^eta * 1/(1+prtp)^(t-year) for (i,t) in enumerate(model_years) if year<=t<=last_year)...]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants