-
Notifications
You must be signed in to change notification settings - Fork 182
DOC: Reword description about patching in preview mode #2491
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
DOC: Reword description about patching in preview mode #2491
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Flags with carried forward coverage won't be shown. Click here to find out more. see 37 files with indirect coverage changes 🚀 New features to boost your workflow:
|
@@ -29,8 +29,9 @@ for being enabled by default for all users: | |||
This type of functionality is available under **preview mode** of |sklearnex| and located in | |||
the corresponding module (`sklearnex.preview`). | |||
|
|||
Preview functionality *may* or *may not* participate in patching of Scikit-learn. | |||
For example, a preview estimator may be a replacement for a stock one or a completely new one. | |||
Functionalities under preview will be made available after patching when preview mode is enabled, |
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.
Are we sure that the sklearnex-only estimators don't get patched-in? https://github.com/uxlfoundation/scikit-learn-intelex/blob/main/sklearnex/dispatcher.py#L62 It takes the mapping without preview, and then adds the preview estimators on top, and for the sklearnex-only estimators, if it didn't patch in, we would not have sklearn conformance deselections for those estimators (mostly for test_common.py
). Maybe I misunderstood? Let me know. This PR should be quick to approve once we iron this out (figure out what I got wrong here).
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.
Yes, looks like patching still makes them importable from sklearn
. Modified the docs here and in the section about extension estimators.
/azp run Docs |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Somehow the Docs CI has failed, and started a re-run. Can be merged if it comes out as passing.
Description
This PR rewords the documentation about patching in preview mode to make it less ambiguous.
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing