Skip to content
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

[MRG+1] remove __future__ imports #12791

Merged
merged 15 commits into from
Feb 2, 2019
Merged

Conversation

surgan12
Copy link
Contributor

@adrinjalali adrinjalali changed the title unimport remove __future__ imports Dec 15, 2018
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

LGTM. IMO There are some unnecessary white lines.

@@ -6,7 +6,6 @@
# Anthony Di Franco (projected gradient, Python and NumPy port)
# License: BSD 3 clause

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whiteline?

@@ -40,7 +40,6 @@
[1] https://archive.ics.uci.edu/ml/datasets/Covertype

"""
from __future__ import division, print_function

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whiteline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Man !

@@ -12,7 +12,6 @@
# All configuration values have a default; values that are commented out
# serve to show the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whiteline?

@@ -34,7 +34,6 @@ class :class:`sklearn.linear_model.Lasso`, that uses the coordinate descent
the circular artifact separating the pixels in the corners, that have
contributed to fewer projections than the central disk.
"""
from __future__ import division

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whiteline?

@@ -6,7 +6,6 @@
Shows how shrinkage improves classification.
"""

from __future__ import division

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary (double) whiteline?

@@ -7,7 +7,6 @@
# License: BSD 3 clause


from __future__ import division, print_function

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary (multiple) whiteline?

@@ -5,7 +5,6 @@
# Copyright: 2010, Gael Varoquaux
# License: BSD 3 clause

from __future__ import division

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whiteline?

@@ -5,7 +5,6 @@
# Author: Florian Wilhelm <[email protected]>
# License: BSD 3 clause

from __future__ import division, print_function, absolute_import

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary whiteline?

@surgan12
Copy link
Contributor Author

@adrinjalali u there ?

@adrinjalali
Copy link
Member

@surgan12 please be patient with our responses. Sometimes it takes us a while. It's not ideal, but response latency can be horribly high here. However, I'm looking at the logs. Somehow as you mentioned, removing the division is kinda breaking all the calculations, I need to figure out why.

@surgan12
Copy link
Contributor Author

surgan12 commented Dec 16, 2018 via email

@adrinjalali
Copy link
Member

@surgan12 could you please merge master? The doc build issue is fixed now there.

@rth
Copy link
Member

rth commented Jan 5, 2019

So we need to resolve #12906 to make CI green here.

@@ -40,7 +40,6 @@
ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
THE POSSIBILITY OF SUCH DAMAGE.
"""
from __future__ import division, print_function, absolute_import
Copy link
Member

Choose a reason for hiding this comment

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

Please revert all changes under sklearn/externals: this is vendored code and it's better to keep it in its original state.

@rth
Copy link
Member

rth commented Jan 5, 2019

Interestingly "Remove __future__ import" commit in #12928 passes CI fine, also I don't see these failures on master.

could you please merge master? The doc build issue is fixed now there.

[commit] Merge branch 'docs_change'

That commit should have said "Merge branch 'master'" I think. Could you please try again @surgan12 ?

Something along the lines of,

git fetch upstream
git merge upstream/master

where upstream points to scikit-learn/scikit-learn.git not your fork.

@adrinjalali
Copy link
Member

@rth the tests passed on your PR cause you were not touching examples (I guess). The issue is that the circle-ci on master somehow doesn't actually do a clean complete run. I suspect the tests on master pass because of this line, but not sure.

@rth
Copy link
Member

rth commented Jan 5, 2019

circle-ci on master somehow doesn't actually do a clean complete run

As far as I know, only dataset download are cached in .circleci/config.yml here and it spins a new docker container for each run, so assuming all docs get build on master (which seem to be the case) it should do a clean and complete run.

The line you mention in the Makefile was more intended for re-running things locally, it should have no effect on CircleCI.

@adrinjalali
Copy link
Member

@surgan12 could you do another update from the master? The issue should be solved now.

@adrinjalali
Copy link
Member

ping @surgan12 , would you like someone else take over this one?

@surgan12
Copy link
Contributor Author

@adrinjalali sorry , this just slipped out of ma head, I m on it.

@adrinjalali
Copy link
Member

I think you may have missed these ones:

sklearn/feature_extraction/tests/test_text.py:from __future__ import unicode_literals
sklearn/model_selection/_split.py:from __future__ import print_function
sklearn/model_selection/_split.py:from __future__ import division

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

thanks @surgan12 !

@adrinjalali
Copy link
Member

@rth I think this is finally ready to be merged.

@adrinjalali adrinjalali changed the title remove __future__ imports [MRG+1] remove __future__ imports Feb 1, 2019
@surgan12
Copy link
Contributor Author

surgan12 commented Feb 1, 2019

@adrinjalali , yep finally ready !!

@@ -1,4 +1,3 @@
from __future__ import print_function, division, absolute_import

Copy link
Member

Choose a reason for hiding this comment

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

please double check that there's no redundant blank line (e.g., blank line at the beginning of a file).

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

I think this is finally ready to be merged.

My comment from #12791 (comment) still needs to be addressed I think.

@surgan12
Copy link
Contributor Author

surgan12 commented Feb 2, 2019

@rth on it !!

@@ -1,5 +1,4 @@
from __future__ import print_function, division, absolute_import

Copy link
Member

Choose a reason for hiding this comment

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

More changes to externals that should be reverted.

If you search for "externals" in the diff on Github that should not contain any files from sklearn/externals.

You can use

git checkout master -- <file_path>

to revert a file to the version from master.

@qinhanmin2014 qinhanmin2014 merged commit 62d2059 into scikit-learn:master Feb 2, 2019
@qinhanmin2014
Copy link
Member

thanks @surgan12

@surgan12
Copy link
Contributor Author

surgan12 commented Feb 2, 2019

@qinhanmin2014 Was good experience contributing for the first time .

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 6, 2019
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 7, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
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.

None yet

5 participants