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

Check that id_loan is not duplicated #164

Merged
merged 8 commits into from
Aug 6, 2020
Merged

Check that id_loan is not duplicated #164

merged 8 commits into from
Aug 6, 2020

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Aug 5, 2020

This relates to a bug identified by multiple users, where projected emission_factors were unreasonably large.

This was a result of the input data having duplicate id_loans per loan. The result was that, while the loan weights was calculated per id_loan, the emission_factor was summed across each loan weight, causing double-counting (sometimes more).

I decided to go the more harsh route, and cause the function to error if input id_loans were duplicated. This will save us headache down the road, as these ids should uniquely identify EVERY loan (otherwise we're gonna have problems).

I also noticed that the emission_factor was still being weighted when by_company = TRUE which doesn't make sense, so fixed that as well.

Maybe closes #160

@jdhoffa jdhoffa requested a review from maurolepore August 5, 2020 15:02
R/utils.R Show resolved Hide resolved
Copy link
Contributor

@maurolepore maurolepore left a comment

Choose a reason for hiding this comment

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

Looks good. I suggested a change and I plan to do it myself.

Copy link
Contributor

@maurolepore maurolepore left a comment

Choose a reason for hiding this comment

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

Thanks Jackson, I approve. If appropriate, please remember to update NEWS.md; and to close the relevant issue(s) -- maybe via the commit message when you squash-merge.

@jdhoffa jdhoffa merged commit 555b12e into RMI-PACTA:master Aug 6, 2020
jdhoffa added a commit that referenced this pull request Aug 6, 2020
@jdhoffa jdhoffa deleted the 160-duplicated-id_loan-bug branch August 6, 2020 08:37
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.

projected and target_* values for steel output of target_sda() seems off
2 participants