Pick up cans down the road

[jacobvjk]

relates to #213

• removes obsolete TODOs
  • # TODO: allow surfacing the other match_name args --> no plans to do so
  • # TODO: when benchmarks get re-introduced, they need to be removed here --> no plans to reintroduce benchmarks for net alignment metric. In case we want to do so in the future, we should reconsider the implementation more broadly
  • # TODO: Note that this implies that no groups across different .by variables should have the same values, as this will confuse output directories --> this was effectively handled by output sub directory for every user facing function #179, because every additional run with a new by_group variable is either going to be written to a new directory (hence there will be no confusion of equally named sub-directories), or it will replace the previous output directory entirely (hence again no naming conflicts for equally named sub groups created in different runs)
  • # TODO: check if this needs to be adjusted to remove other by_group columns the extended implementation of by_group (enable by_group for run_pacta #108, Enable by_group in calculate_match_success_rate() #120, Enable by group lbk coverage #127) has not lead to any issues with generating outputs
  • # TODO: decide if this should be removed from outputs --> no pressing need to remove a column that indicates the calculation was run at meta level
• relevant TODOs have received their own GH issues (rework output type data_scatter_sector #248, consider deprecating output companies_included #249, rework output format of lbk_match_success_rate #250)
• close TODOs related to config validations by adding the relevant assertions

Note

TODOs related to the plots, tests, and examples in the user-facing functions will be handled in another PR

[codecov]

Codecov Report

Attention: Patch coverage is 90.32258% with 6 lines in your changes missing coverage. Please review.

Project coverage is 52.07%. Comparing base (2a2a706) to head (1be8f44).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
R/match_loanbooks.R	50.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
+ Coverage   51.35%   52.07%   +0.71%     
==========================================
  Files          28       28              
  Lines        3102     3161      +59     
==========================================
+ Hits         1593     1646      +53     
- Misses       1509     1515       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[cjyetman]

all looks reasonable to me, though to be honest there's a lot going on here so it's hard for me to fully tell if some things were properly addressed or not, e.g. this comment was removed "# TODO: when benchmarks get re-introduced, they need to be removed here", but it's not clear to me if/where that is be handled elsewhere in this PR

[jacobvjk]

all looks reasonable to me, though to be honest there's a lot going on here so it's hard for me to fully tell if some things were properly addressed or not, e.g. this comment was removed "# TODO: when benchmarks get re-introduced, they need to be removed here", but it's not clear to me if/where that is be handled elsewhere in this PR

yes, I realize that if we want to track all of these properly, I would have to make one Pr for each type of TODO. the one you mentioned was removed as obsolete because there is no plan on the horizon to reintroduce corporate economy benchmarks for the net alignment metric

[jacobvjk]

I added a more detailed description to the PR to ensure the reasoning for every removal can be tracked