Skip to content

feat: Support sample weight for MLP #25646

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 45 commits into from
Closed

feat: Support sample weight for MLP #25646

wants to merge 45 commits into from

Conversation

zshu115x
Copy link

@zshu115x zshu115x commented Feb 19, 2023 •
edited
Loading

Reference Issues/PRs

sample_weight support will contributes to class_weight support, so this PR contributes to the issue #9113, and the stalled PR #11723

What does this implement/fix? Explain your changes.

  • Add sample_weight support for MLP

    • Add sample_weight support for multiple loss functions:
      • log_loss
      • binary_log_loss
      • squared_loss
    • Add sample_weight parameter to fit and partial_fit
      • Propagate to relevant functions
      • Apply sample_weight to gradient calculation
      • Validate sample_weight
      • Handle zero weights
    • Add tests
      • Test directly loss functions sample_weight invariance
      • Test fit validation of sample_weight
      • Test sample weight effect for both MLPClassifier and MLPRegressor
      • Test sample weight together with early stopping

Any other comments?

I have a draft PR #25326 to support class weight for MLP which will depend on this PR for sample weight support first.

@zshu115x zshu115x marked this pull request as ready for review February 19, 2023 23:36
@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch from da3b30b to 6363d29 Compare February 20, 2023 15:16
@zshu115x zshu115x mentioned this pull request Feb 24, 2023
Open
15 tasks
@zshu115x
Copy link
Author

Hi @jnothman @dorcoh @thomasjpfan, moving the comment #11723 (comment) here... Could you please review this PR to add sample weight support for MLP as the prerequisite for adding class weight support?

(I am working on another PR to support class weight #25326)

@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch from 9a66842 to 492c906 Compare March 5, 2023 04:18
thomasjpfan
thomasjpfan reviewed Mar 20, 2023
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR! I left a review, but I do not have the bandwidth to quickly follow up.

@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch 3 times, most recently from c9fa4ad to bfd035c Compare March 31, 2023 04:19
@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch 2 times, most recently from 95b07e6 to 698c9ce Compare April 3, 2023 15:34
@zshu115x zshu115x mentioned this pull request Apr 16, 2023
Draft
15 tasks
@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch from 698c9ce to 6159492 Compare April 16, 2023 20:54
@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch from 6159492 to e2ff6b7 Compare April 26, 2023 04:33
@adrinjalali
Copy link
Member

The MLP classes in scikit-learn are mostly for educational purposes. Do we want to introduce sample weights here, which also introduces more maintenance burden? We have mostly decided not to add features to the MLP classes in the past.

@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch from 0fc896f to 1bfee0f Compare May 14, 2023 15:38
@zshu115x zshu115x requested a review from thomasjpfan May 14, 2023 18:55
@zshu115x
Copy link
Author

The MLP classes in scikit-learn are mostly for educational purposes. Do we want to introduce sample weights here, which also introduces more maintenance burden? We have mostly decided not to add features to the MLP classes in the past.

Good to know that MLP is mostly for educational purposes in scikit-learn. Then it makes sense to not add additional features in MLP here. I planned to add class weight support as well, but I think I will skip it.

The implementation here to support sample weight is pretty straightforward and clean, so I hope it doesn't introduce more maintenance burdens. And I think it will be a useful feature even just for educational purposes.

I have addressed previous comments and enhanced the implementation and test cases for this PR. Do you think we can move forward with it?

@zshu115x
Copy link
Author

Hi @thomasjpfan, thanks a lot for your comments! I have addressed most of your comments. Could you retake a look at this PR? Thanks.

zshu115x
zshu115x commented May 14, 2023
View reviewed changes
@EwoutH
Copy link

EwoutH commented May 23, 2024

I would really love to have this feature in scikit learn!

@adrinjalali
Copy link
Member

@scikit-learn/core-devs what do we think here? It's in a nice state, do we want to get it to the finish line or close?

@lorentzenchr
Copy link
Member

do we want to get it to the finish line

+1

@jjerphan
Copy link
Member

I think it makes sense since #9113 is the most upvoted issue.

@OmarManzoor
Copy link
Contributor

I think it might be a good idea to complete this given that considerable work has been put in.

lorentzenchr
lorentzenchr reviewed May 23, 2024
View reviewed changes
@glemaitre glemaitre self-requested a review May 24, 2024 08:36
@glemaitre glemaitre added this to the 1.6 milestone May 24, 2024
@EwoutH
Copy link

EwoutH commented Jun 3, 2024

@zshu115x are you still interested in working on driving this PR home?

zach-shu and others added 27 commits November 7, 2024 08:19
@zach-shu @zshu115x
fix: apply sample weight to validation data
2ceee1e
@zach-shu @zshu115x
chore: update doc
92a5738
@zach-shu @zshu115x
test: update a unit test
7364d84
@zach-shu @zshu115x
fix: fix a variable reference issue
af48101
@zach-shu @zshu115x
fix: fix sample_weight implementation
86ecaf4
@zach-shu @zshu115x
fix: raise an expcetion when sample_weight is all zeros
1f78079
@zach-shu @zshu115x
test: fix unit tests for sample weights
ffe81d1
@zach-shu @zshu115x
fix: fix a lint compliant
05f7fcb
@zach-shu @zshu115x
test: add additional tests for code coverage
f85b812
@zach-shu @zshu115x
fix: revert an unnecessary change
feb1a60
@zach-shu @zshu115x
fix: rebase feature branch and address comments
9bef5b2
@zach-shu @zshu115x
fix: fix sample weight support for neural networks
f379bdc
@zach-shu @zshu115x
chore: revert a doc change
b0f696f
@zach-shu @zshu115x
fix: address comments
cfd13ca
@zach-shu @zshu115x
chore: fix a doc format complaint
7aeb1d3
@zshu115x
fix: rebase and fix conflicts
e71ce77
@zshu115x
fix: update changelog
b3a08bf
@zshu115x
fix: fix changelog
7d6e8d3
@zshu115x
fix: fix the score function and the use of validate_data()
a38e50d
@zshu115x
fix: uncheck sample_weight_equivalence check for MLP
5276fd9
@zshu115x
fix: update xfail_check tags
57b11f2
@zshu115x
fix: add sklearn tags to MLPRegressor
8746526
@zshu115x
fix: remove wrong field
ebecf90
@glemaitre @zshu115x
revert unecessary change
5d2b8e3
@zshu115x
fix: address comments and fix a few typos and format issues
73ffb3e
@zshu115x
fix: improve two test cases
115fe80
@zshu115x
fix: remove unnecessary new lines
ac08865
@zshu115x zshu115x force-pushed the support_sample_weight_mlp branch from be0f76d to ac08865 Compare November 7, 2024 13:19
@lorentzenchr
Copy link
Member

@zshu115x First of all, I'm really grateful for your work on this PR. Don't get the wrong impression. Then, with the approaching 1.6 release, I thought it would good to have this feature and because of a few months of silence (no reason to appologize), I opened my PR where I took a lot from this PR!

Then on the matter of regularization:

  • There is no correct way of doing it.
  • My personal preference is always 1/sum(sw) * sum(sw * loss) + alpha * penalty. So the penalty does not depend on the samples/sample size.
  • As MLP already has the given setup without sw, namely like Ridge - the reason is that this might give a better default value for alpha - I prefer the scaling of my PR as I already mentioned. AFAIK, this is again the same one that Ridge uses.

I close here as I don't think it is fruitful to discuss any further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@adamjstewart adamjstewart adamjstewart left review comments

@OmarManzoor OmarManzoor OmarManzoor approved these changes

@thomasjpfan thomasjpfan Awaiting requested review from thomasjpfan

@lorentzenchr lorentzenchr Awaiting requested review from lorentzenchr

Assignees
No one assigned
Labels
module:neural_network
Projects
Community feature request
Status: Done
Milestone
1.6
Development

Successfully merging this pull request may close these issues.
10 participants
@zshu115x @adrinjalali @EwoutH @lorentzenchr @jjerphan @OmarManzoor @adamjstewart @thomasjpfan @glemaitre @zach-shu