Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

33 add optional arg .by for grouping #34

Merged
merged 12 commits into from
Apr 24, 2024
Merged

33 add optional arg .by for grouping #34

merged 12 commits into from
Apr 24, 2024

Conversation

jacobvjk
Copy link
Member

@jacobvjk jacobvjk commented Apr 9, 2024

closes #33
closes #21

  • calculation of net alignment metric does not depend on any grouping information (previously depended on group_id, which represented loan books)
  • for all functions in calculate_company_alignment_metric.R this means they now truly return company level information without any references to loan books or other groups previously implied by group_id
  • aggregate_alignment_loanbook_exposure() gains argument .by which can be used to define the level of aggregation for loan books
    • Default is NULL which means that aggregation happens without grouping. This corresponds to the meta loan book when multiple loan books are processed.
    • .by accepts character vectors as inputs. Providing more than one variable to group by is possible
    • the groups must be available in matched_prioritized

@jacobvjk
Copy link
Member Author

@jdhoffa I think this PR now covers what we want to achieve here. Given the extent of the changes here, I am keeping it as a draft for now and would ask for your initial review. I expect there are probably a few things that could be changed cosmetically that we may want to add as well

@jacobvjk jacobvjk requested a review from jdhoffa April 15, 2024 15:39
Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

A check-point review and some requested changes

Let me know when this is ready for a more in-depth review and I will test it out in more detail locally

R/aggregate_alignment_loanbook_exposure.R Outdated Show resolved Hide resolved
R/aggregate_alignment_loanbook_exposure.R Show resolved Hide resolved
R/aggregate_alignment_loanbook_exposure.R Show resolved Hide resolved
R/calculate_company_alignment_metric.R Show resolved Hide resolved
@jdhoffa
Copy link
Member

jdhoffa commented Apr 16, 2024

NIT: maybe also change the name of the PR, since the argument is now called .by?

@jacobvjk jacobvjk changed the title 33 add group var 33 add optional arg .by for grouping Apr 17, 2024
@jacobvjk jacobvjk marked this pull request as ready for review April 17, 2024 11:20
@jacobvjk jacobvjk requested a review from jdhoffa April 18, 2024 15:58
Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

lgtm!! 🚀

@jacobvjk jacobvjk merged commit 2a04b63 into main Apr 24, 2024
15 checks passed
@jacobvjk jacobvjk deleted the 33-add-group-var branch April 24, 2024 16:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants