Skip to content

from __future__ import: not needed in Python 3 #21161

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

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

DimitriPapadopoulos
Copy link
Contributor

Reference Issues/PRs

#21153

What does this implement/fix? Explain your changes.

Reopen #21153. While these files originate in SciPy, they have been removed from SciPy
and are now maintained here. So they are not vendored files any more.

Any other comments?

@glemaitre
Copy link
Member

Instead of removing just the import, we should copy-paste the full file in case that some bug fixes have been applied there.

@DimitriPapadopoulos
Copy link
Contributor Author

@glemaitre I'm agree to copy if possible, but from where? Again, while these files originate in SciPy, they have been removed from SciPy and are now maintained here.

See initial PR #10427.

See also previous history of these files, they have already been modified multiple times:
https://github.com/scikit-learn/scikit-learn/commits/main/sklearn/externals/_lobpcg.py
https://github.com/scikit-learn/scikit-learn/commits/main/sklearn/externals/_pilutil.py

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I had not realized those where orphaned vendored files.

Ok for this import clean-up. We should probably a find a better solution for the long term maintenance of those files because having them in externals is misleading.

Either vendoring alternative implementations that are maintained out of scipy if they exist or importing a replacement from scipy, or moving to sklearn/utils, outside of externals.

But this should be done in dedicated PRs, separately for _lobpcg.py and _pilutil.py.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Sep 27, 2021

While scipy/misc/pilutil.py had been removed in scipy/scipy#9325, I can still find a scipy/sparse/linalg/eigen/lobpcg/lobpcg.py file. Let me double-check that file.

I'll move _pilutil.py to sklearn/utils in a different PR.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Sep 27, 2021

Actually scipy/sparse/linalg/eigen/lobpcg/lobpcg.py starts with:

"""
scikit-learn copy of scipy/sparse/linalg/eigen/lobpcg/lobpcg.py v1.3.0
to be deleted after scipy 1.3.0 becomes a dependency in scikit-lean
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

This file was supposed to removed for SciPy 1.3.0, but it's actually still there! I might open an issue in SciPy about it. I'll copy the latest version here in an upcoming PR.

@DimitriPapadopoulos
Copy link
Contributor Author

I have created #21163 to update _lobpcg.py since it's still there. I'll rework this PR.

@glemaitre
Copy link
Member

I might open an issue in SciPy about it. I'll copy the latest version here in an upcoming PR.

No need to be open an issue. The comment is still meaningful. Our minimum requirement is SciPy 1.1.0 that is lower than 1.3.0 when lobpcg was introduced in SciPy. We still need to wait until we bump the version. However, we should copy the current SciPy version into our externals file.

@glemaitre glemaitre added this to the 1.0.1 milestone Sep 27, 2021
@glemaitre
Copy link
Member

I think that we should tag this PR for 1.0.1 to have the latest lobpcg in our release indeed. I don't know if it includes some bug fixes thought.

@DimitriPapadopoulos
Copy link
Contributor Author

Ah, got it! I thought that file was supposed to be deleted in SciPy 1.3.0, not introduced in SciPy 1.3.0.

"""
scikit-learn copy of scipy/sparse/linalg/eigen/lobpcg/lobpcg.py v1.3.0
to be deleted after scipy 1.3.0 becomes a dependency in scikit-lean
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

@DimitriPapadopoulos
Copy link
Contributor Author

I think that we should tag this PR for 1.0.1 to have the latest lobpcg in our release indeed. I don't know if it includes some bug fixes thought.

Yes, I think there were some bugs (see maxIteration vs. maxiter).

While these files originate in SciPy, they have been removed from SciPy
and are now maintained here.
@DimitriPapadopoulos
Copy link
Contributor Author

I have changed this PR to update _pilutil.py only.

The other file, _lobpcg.py, is updated in #21163.

@ogrisel
Copy link
Member

ogrisel commented Sep 27, 2021

I think that we should tag this PR for 1.0.1 to have the latest lobpcg in our release indeed. I don't know if it includes some bug fixes thought.

I don't like doing this kind of change in a bugfix release. I think it's fine to do the cleanup later.

Edit: if there are some bugs, then ok to consider it a bug fix but it should be documented as such in the 1.0.1 changelog.

@ogrisel
Copy link
Member

ogrisel commented Sep 27, 2021

Looking at the diff, it does not seem like a bug fix, only a renaming of maxIteration to maxiter and a fix in the docstring of the function (which is not part of the scikit-learn public API).

@glemaitre
Copy link
Member

Looking at the diff, it does not seem like a bug fix, only a renaming of maxIteration to maxiter and a fix in the docstring of the function (which is not part of the scikit-learn public API).

So we can post-pone for 1.1 release. We will not need any changelog even.

@glemaitre glemaitre modified the milestones: 1.0.1, 1.1 Sep 27, 2021
@ogrisel
Copy link
Member

ogrisel commented Sep 27, 2021

+1 for merging #21163 as is. And I am still +1 for merging this PR as is.

@glemaitre
Copy link
Member

Thanks @DimitriPapadopoulos

@DimitriPapadopoulos DimitriPapadopoulos deleted the scipy_externals branch September 27, 2021 12:38
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants