-
Notifications
You must be signed in to change notification settings - Fork 112
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
Allow MapieRegressor
to use K-fold iterator variants with stratification and groups.
#393
Allow MapieRegressor
to use K-fold iterator variants with stratification and groups.
#393
Conversation
Hello @pidefrem ! Thank you for this contribution. I'll take the time to review it. Just to be in line with your proposal, I understand that in Don't hesitate to contact me if you need any help. |
Hello @thibaultcordier, yes it refers to the split methods of the cross validator used during the fit of the MAPIE estimators. Please feel free to suggest any other description that you think is more suitable. |
MapieRegressor
to use group split strategyMapieRegressor
to use K-fold iterator variants with stratification and groups.
17b0bf3
to
7842773
Compare
…n _appserveradm _lpadmin awagent_enrolled com.apple.sharepoint.group.1 _appstore _lpoperator _developer _analyticsusers com.apple.access_ftp com.apple.access_screensharing com.apple.access_ssh com.apple.access_remote_ae com.apple.sharepoint.group.2 arg is at the end to avoid breaking change
dc21696
to
98fadea
Compare
@thibaultcordier I fixed some issues and added some tests, tell me if it is ok now. |
Hello @pidefrem, I'll check your changes this week. Thank you for contacting me about the review. I'll keep you informed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pidefrem Great! Thank you for these changes, I don't have any feedback to give, ready to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pidefrem,
Thank you for this PR! This will be extremely useful. One little modification would be to remove the environment from the MAPIE folder/GitHub. It can be saved outside the folder.
If you can do this, we will be able to merge your PR asap!
Thank you again!
Makefile
Outdated
lint: | ||
flake8 . --exclude=doc,.venv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pidefrem,
Could you please remove the environment? You can also save the env somewhere else than in the MAPIE folder.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little modification would be to remove the environment from the MAPIE folder/GitHub. It can be saved outside the folder.
Description
Continuing the work done in PR
Allow the use of
GroupKFold
cv-split (see https://scikit-learn.org/stable/modules/generated/sklearn.model_selection.GroupKFold.html) and also the use of custom cv-splits based onStratifiedKFold
for example (see https://scikit-learn.org/stable/modules/generated/sklearn.model_selection.StratifiedKFold.html#sklearn.model_selection.StratifiedKFold)Fixes #202
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist
make lint
make type-check
make tests
make coverage
make doc