-
-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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
ENH Adds lazy module at the top level #29793
ENH Adds lazy module at the top level #29793
Conversation
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.
I think the added complexity/magic is minimal and the ability to do a single import sklearn
line in an IPython session followed by code composing classes looked from different modules by qualnames is quite convenient.
So +1 on my side.
I am not sure about how to describe this in the changelog entry, but maybe this is better to introduce this with an example in the release highlights.
Before merging this PR, we might want to get more feedback than usual though, either by more reviews / votes or by validating it at the next monthly dev meeting.
EDIT: __getattr__
magic (even if quite straightforward in this case) and introducing multiple usage/import styles for sklearn
make me ponder the above. So I am +0 instead of +1.
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.
LGTM. I like that now ipython can auto complete sklearn submodules.
doc/whats_new/v1.6.rst
Outdated
- |Enhancement| Scikit-learn classes and functions can be imported from the root | ||
`sklearn` module. :pr:`29793` by `Thomas Fan`_. |
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.
I find the formulation a bit misleading. One can think that he can now do from sklearn import LogisticRegression
.
For me a key was to think about this PR as enabling a new/different style of Python, and not so much "lazy loading to make imports faster". I'm undecided if I like adding the "new style" or not. Not so much because I like/dislike the technical implementation but more because it adds a different way of doing the same thing. And on top of that I don't experience the problem that this solves. |
When working in interactive IPython sessions, it often takes as many import statements as useful code lines to reproduce a toy experiment interactively to prove a point during an educational live coding session or craft a minimal reproducer. |
Actually I like the point raised by @ogrisel. This is indeed something that I experienced. |
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.
Other than the changelog, LGTM.
Co-authored-by: Adrin Jalali <[email protected]>
doc/whats_new/v1.6.rst
Outdated
@@ -35,6 +35,9 @@ Changes impacting many modules | |||
More details in :ref:`estimator_tags`. | |||
:pr:`22606` by `Thomas Fan`_ and :pr:`29677` by `Adrin Jalali`_. | |||
|
|||
- |Enhancement| Scikit-learn classes and functions can be used while only having a |
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.
You will need to add a fragment in doc/whats_new/upcoming_changes/many-modules/29793.enhancement.rst
and have the following entry:
- Scikit-learn classes and functions can be used while only having a
`import sklearn` import line. For example, `import sklearn; sklearn.svm.SVC()` now works.
By :user:`Thomas Fan <thomasjpfan>`
Reference Issues/PRs
Simpler version of #26440
What does this implement/fix? Explain your changes.
This PR implements lazy loading at the top level. With this PR, it enables users to write code like this: