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

Use only either sector or sector_ald #178

Closed
jdhoffa opened this issue Aug 14, 2020 · 0 comments · Fixed by #179
Closed

Use only either sector or sector_ald #178

jdhoffa opened this issue Aug 14, 2020 · 0 comments · Fixed by #179

Comments

@jdhoffa
Copy link
Member

jdhoffa commented Aug 14, 2020

There are two "sector defining columns" in the match_result input: sector and sector_ald.

  1. sector comes from the loanbook specification
  2. sector_ald comes from the asset-level data

In the analysis, we should only ever use the actual data in the sector_ald column. From what I can tell, this is already happening, however, internally, we are joining and renaming and abusing the word sector when we mean sector_ald.

I think we should decide on a consistent method, so that we don't confuse people (and ourselves).
Options:

  1. Drop sector as soon as data is input, and rename sector = sector_ald
  2. Try to keep track of sector_ald and not accidentally reference the wrong column
@jdhoffa jdhoffa added this to To do in r2dii via automation Aug 14, 2020
@jdhoffa jdhoffa moved this from To do to In progress in r2dii Aug 14, 2020
r2dii automation moved this from In progress to Done Aug 14, 2020
maurolepore pushed a commit that referenced this issue Aug 14, 2020
…tasets (#179)

Closes #178

We confused sector and sector_ald in a few places.
This was effectively a silent error, since we are only 
allowing for `r2dii.match::match_name(..., by_sector = TRUE)`.
@AlexAxthelm AlexAxthelm removed this from Done in r2dii Sep 11, 2022
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 a pull request may close this issue.

1 participant