Skip to content

Conversation

@henrydingliu
Copy link
Collaborator

Address #623

  • Added drop and drop_valuation to ML methods
  • Added helper function to recreate stepwide patsy formula
  • Various tests

henrydingliu and others added 13 commits December 30, 2025 22:41
sample weights enables dropping specific points from fitting, which is essential for recreating BZ results
- separating weight flattening into a separate method from _prep_X_ml
- adding sample weight support to glm
- cleaning up how weights are handled in each ml method.
- various fixes per #533
removing feat_eng from developmentML
* Fix for #634 (#638)

squashed commits as some the earlier commit were later reversed

* Added exposure adjustment to barnzehn.py. Updated ptf example gallary with exposure adjustments to match paper.

* fixed bug when not passing sample_weight to BarnettZehnwirth.fit. updated plot_ptf_resid in gallery.

* removed sample_weight from barnettzehnwirth.fit

* unremoved sample_weight from barnettzehnwirth.fit -- the estimator ignores it as it did originall

* fixed graph title

---------

Co-authored-by: danielfong-act <[email protected]>

* adding another test for bz

---------

Co-authored-by: danielfong-act <[email protected]>
Co-authored-by: danielfong-act <[email protected]>
* Initial PTF_formula commit.

* origin trend groups (alpha) are passed as tuples denoting ranges to PTF_formula. Removes first year's origin trend group, if specified (intercept term takes its place).

* Rmoved mostly useless multicollinearity warning. Users should exercise caution in genral

* makes development/tests/test_barnzehn use PTF_formula to construct the formula for test_bz_2008. Tweaks alpha parameters to be additive again.

---------

Co-authored-by: danielfong-act <[email protected]>
@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 91.37931% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.05%. Comparing base (d4b2c42) to head (79231bf).
⚠️ Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
chainladder/utils/utility_functions.py 66.66% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
+ Coverage   84.44%   85.05%   +0.60%     
==========================================
  Files          84       85       +1     
  Lines        4823     4865      +42     
  Branches      610      619       +9     
==========================================
+ Hits         4073     4138      +65     
+ Misses        538      518      -20     
+ Partials      212      209       -3     
Flag Coverage Δ
unittests 85.05% <91.37%> (+0.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrydingliu
Copy link
Collaborator Author

@kennethshsu do you want to take a look before we merge into master?

@kennethshsu
Copy link
Collaborator

Can you merge without a review?

How about @danielfong-act as the reviewer? You two have been spearheading this and I don't feel qualified...

@danielfong-act
Copy link
Contributor

danielfong-act commented Jan 9, 2026

@kennethshsu Sure, I'll take another look.
Things look good. I don't have much to add, since the drops are optional and don't affect estimators if they're not supplied. The only new thing I'm noticing on this pass is that there appears to be a small bugfix with the logic of fit_incrementals in learning.py.

@henrydingliu
to address what you mentioned here. I think parameter specification should be simplified, so you could tweak things so that each direction is specified by a list of indices, if you'd like.
Refactoring things to integrate PTF_formula might be worth saving for another PR. We still have to change the documentation and gallery examples, so we can hold off on that kind of design decision until then. This code's been cooking for a while and I'd like to see it merged soon.

@kennethshsu
Copy link
Collaborator

@danielfong-act can you self assign as a reviewer? I can't assign you for some reason.

Copy link
Contributor

@danielfong-act danielfong-act left a comment

Choose a reason for hiding this comment

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

Looks ready to merge, unless you wanted to change tweak PTF_formula's parameters

Edit: yeah I don't see the option to, and submitting a review doesn't change much because I'm a contributor.

@henrydingliu
Copy link
Collaborator Author

henrydingliu commented Jan 9, 2026

@danielfong-act We can raise the refactoring of lists as an enhancement issue :) And the incremental fix is to close #533.

@kennethshsu Daniel doesn't have write access. His review doesn't formally allow the PR to proceed. Could you please approve based on his review and merge into master?

@kennethshsu
Copy link
Collaborator

Sounds good. So this is "approved".

Let me figure it out with Paul to see how we can get the reviewer assigned. I thought I fixed this a while back but probably missed something. @henrydingliu I will copy you on the email. @danielfong-act what's your email? I can add you in if you want.

@hutch3232 hutch3232 requested a review from kennethshsu January 9, 2026 19:11
@hutch3232
Copy link

hutch3232 commented Jan 9, 2026

Hey @kennethshsu,

Do these drop downs not work for you?
Screenshot_20260109-141134

Screenshot_20260109-141147

Since you are an admin of the repo, you should definitely be able to do this. I poked through repo settings and didn't see anything that would limit it.

Edit: doesn't solve the immediate problem necessarily, but could be worth adding a CODEOWNERS file to the repo. Example:
https://github.com/pola-rs/polars/blob/main/.github/CODEOWNERS

Edit2: another possibility - were you looking from the "Files changed" tab? I believe that's the only place you can submit an approval from (not talking about assignment).

@kennethshsu
Copy link
Collaborator

@hutch3232, no, I don't see @danielfong-act available in the drop downs. He's also unable to self-assign neither.

@kennethshsu
Copy link
Collaborator

Does the user need to be a member of the organization?

@hutch3232
Copy link

Oh, apologies, I misunderstood. @danielfong-act is already a reviewer, so he can't be added again.

To satisfy branch protection rules, a member (code owner) must supply an approval. This intentionally makes it so outside contributors can't approve and merge their own PR.

You would need to grant @danielfong-act write permissions (at a minimum) for his approval to "count". That may or may not be what you want. Depends on how small a group actually "owns" the code.

@kennethshsu kennethshsu removed their request for review January 9, 2026 19:45
@kennethshsu
Copy link
Collaborator

@danielfong-act did you do anything? I see that you have officially approved this PR. I didn't add him though, in fact, I couldn't.

@hutch3232 what is your recommendation on the best way to manage this? Do you think we should have a list of users that have write permissions that of course needs to be added and approved by the committee. Or should everyone off the list? My preference is that everyone on the working group belongs to a group, and have write permission. We already have a branch protection rule that an independent reviewer is needed (although I see that I can bypass the rule, which is dangerous, not sure if other people can see that).

@hutch3232
Copy link

@danielfong-act did you do anything? I see that you have officially approved this PR. I didn't add him though, in fact, I couldn't.

I suspect that the approver list drop down only populates with repo members, or perhaps further limited to those with write access. Not 100% sure. Still - I think people can manually submit approvals without being assigned (unless your repo settings explicitly block that).

@hutch3232 what is your recommendation on the best way to manage this?

I don't have too strong of an opinion, but I would recommend keeping just a smaller, core set of people as having write permissions (or greater). The committee members could still easily contribute via forks and wait for a core maintainer to approve + merge. If someone is making a lot of contributions, you could consider elevating their permissions. To be fair, this doesn't seem like a high risk repo to be more permissive... but there also doesn't seem to be a strong need to open it a lot. I'll defer to you and team, though.

although I see that I can bypass the rule

That is because you have admin rights on this repo (and org level - as I could also bypass it).

@jbogaardt-wcf
Copy link
Contributor

Hi guys, I think core contributors who have a deep understanding of the history, goals, and philosophies of the package should review PRs. We can certainly add more core contributors as needed, but I do think the bar should be higher than simply authorizing every member of the committee to merge PRs.

@danielfong-act
Copy link
Contributor

@kennethshsu yeah I sent in a review just to see what would happen. My review went through, but (as expected) it didn't count as a collaborator-level review. I also agree that I don't need write access at this point. PRs seem to be getting addressed by write-level collaborators in a reasonable timeframe.
For future reference, here is my email, which is also on my profile.

@kennethshsu
Copy link
Collaborator

Sounds good, I am going to merge this to main now. Thanks everyone!

Are you on the new working group mailing list? If not, we'd love to have you.

I think the working group needs to decide together how we want the repo handle write privileges. We can discuss this at the next call.

@kennethshsu kennethshsu merged commit aa949dc into master Jan 9, 2026
15 checks passed
@kennethshsu kennethshsu deleted the BZ_w_drop branch January 9, 2026 21:29
@danielfong-act
Copy link
Contributor

@kennethshsu I don't think I am. Please add me, and thanks.

@kennethshsu
Copy link
Collaborator

Hi @HeatherLDavis, can you help add Daniel to our working group? His email is above! Thanks so much!

@HeatherLDavis
Copy link

HeatherLDavis commented Jan 14, 2026 via email

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.

7 participants