-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT move an orphaned vendored file around #21166
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
MNT move an orphaned vendored file around #21166
Conversation
Since this is a file for internal use, does this need to be in the changelog? |
Not sure about the Azure CI |
Yea. |
@thomasjpfan Makes sense. I ran flake8 and black on the file, but some Azure CI jobs still fail. |
The tests are running doctests, which are failing. For example For future reference, you can explore these errors by clicking on details in the failed check: and navigating to the error, which is here Thoughts on this PRFor this PR, I would consider wrapping Pillow lightly, since we only use There are two public functions that use functions in
(This way, we do not need to maintain a big |
I had seen that page, but it doesn't make sense:
However, now that you've pointed me to these doctest messages, I'll try fixing them:
|
File scipy/misc/pilutil.py has been removed from Scipy 1.3.0, see scipy/scipy#9325. Move _pilutil.py from sklearn/externals, which is the location for vendored files, to sklearn/utils, which is a location for locally maintained files, including orphan vendored files like this one.
Either import the private module documented in these examples, or we remove the examples altogether. I chose to keep the examples.
037c9ab
to
c7f5e84
Compare
It looks like doctests are OK after my last commit. Please let me know if anything else is needed. About wrapping Pillow lightly: nothing against it, but this change feels more involved than the current PR. I would rather defer such a change to another PR – provided I can find the time. |
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.
Looking at this again, I'm okay with the moving the vendored file into utils
for now.
LGTM
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.
Thank you, @DimitriPapadopoulos.
I am mostly OK to move this file under utils
for maintenance semantic reasons since it has been deprecated in Scipy. (I think having as little vendored code as possible is good for maintenance.)
IMO, it looks like the implementations can be simplified, but that's out of the scope of this PR (and it is also probably low priority as a change).
Edit: I would wait for a third reviewer before merging this one.
I think we can move to removing pilutils altogether
After looking at this again, I prefer removing the file all together. I opened #22743 as an alternative, which removes |
I would also lean towards the alternative implemented in #22743. |
Closing since #22743 was merged. |
Reference Issues/PRs
See also #21161 and #21165.
What does this implement/fix? Explain your changes.
File
scipy/misc/pilutil.py
has been removed from Scipy 1.3.0, see scipy/scipy#9325.Move
_pilutil.py
fromsklearn/externals
, which is the location for vendored files, tosklearn/utils
, which is a location forlocally maintained files, including orphan vendored files like this one.
Any other comments?
This is a copy of #21165. Something went wrong while rebasing.