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

Fill NAs with zero in abcd #460

Merged
merged 8 commits into from
Feb 5, 2024
Merged

Fill NAs with zero in abcd #460

merged 8 commits into from
Feb 5, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Jan 31, 2024

This PR adjusts the expected behaviour of target_market_share in how it handles input abcd data when abcd$production contains NAs. Previously it would filter these rows and remove from analysis. Now, it replaces them with 0.

@jacobvjk I have a follow-up question, which is, do we want target_sda to exhibit the same behaviour? I imagine probably we do.

and @cjyetman I wanted your input here, do you think this is a reasonable thing to do? (see linked issue for more reference). I know there are, in general, fears around replacing NAs with 0s willy nilly, but I think in this particular case, we do actually expect that PAMS data with NA production values DO mean 0 production... Especially if the asset has not "turned on" yet.

Closes #423

@jdhoffa jdhoffa requested review from cjyetman and jacobvjk January 31, 2024 15:09
@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 31, 2024

Note: It seems #455 has introduced a note in R CMD check... it is not complaining that r2dii.match is in "Depends" but not imported or called in the source code. We may want to revisit that...

Since this is only a NOTE, it won't appear in our automated CI/CD, but it's something worth looking into in any case.

Handling this in #461

@cjyetman
Copy link
Member

No strong opinion here because @jdhoffa and @jacobvjk know better what needs to be done here than I do, but general thoughts...

  • replacing NA with 0 can, under some circumstances, significantly change what the data means
  • replacing NA with 0 at the beginning of a time series that has >0 values after it is probably mostly safe, particularly in this context
  • replacing NA with 0 at the end of a time series after >0 values probably has a higher chance of being wrong

@jdhoffa
Copy link
Member Author

jdhoffa commented Jan 31, 2024

Thanks CJ, that was the context I was hoping for! :-)

In that case, I think the gist of this PR is ok, though perhaps the implementation should be a bit more precise... @jacobvjk what do you think

@jacobvjk
Copy link
Member

jacobvjk commented Feb 1, 2024

I still feel like it is reasonable safe to assume that an NA is an asset that is not actually operating at that point in time. In that case a replacement with zero is fine. I remember being told in the data herald that when no updated information on production sites are available, PAMS assumes that the level of production/capacity from the previous year is still valid.

@jacobvjk
Copy link
Member

jacobvjk commented Feb 1, 2024

@jacobvjk I have a follow-up question, which is, do we want target_sda to exhibit the same behaviour? I imagine probably we do.

I am not so sure about this one. imagine you have a steel plant that is not operational in a given year (has either 0 or NA production). I would expect that steel plant to have an NA value for emission intensity. clearly the technology employed would not be a zero carbon technology even though nothing is produced. so I think you can't replace the NA with a 0 here. If you did, the exposure to this plant would be considered zero emissions and you could reduce your portfolio's emission intensity by having high financial exposure to an inactive plant.

Am I understanding your point right? @jdhoffa

@jdhoffa
Copy link
Member Author

jdhoffa commented Feb 1, 2024

Makes sense! And agreed.

Then I think this PR is good to go

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

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.

Potential Bug: Handling NAs in the ABCD
3 participants