-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
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.
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 | |||
|
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.
unnecessary whiteline?
@@ -40,7 +40,6 @@ | |||
[1] https://archive.ics.uci.edu/ml/datasets/Covertype | |||
|
|||
""" | |||
from __future__ import division, print_function | |||
|
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.
unnecessary whiteline?
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.
Thanks Man !
@@ -12,7 +12,6 @@ | |||
# All configuration values have a default; values that are commented out | |||
# serve to show the default. | |||
|
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.
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 | |||
|
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.
unnecessary whiteline?
examples/classification/plot_lda.py
Outdated
@@ -6,7 +6,6 @@ | |||
Shows how shrinkage improves classification. | |||
""" | |||
|
|||
from __future__ import division | |||
|
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.
unnecessary (double) whiteline?
sklearn/decomposition/nmf.py
Outdated
@@ -7,7 +7,6 @@ | |||
# License: BSD 3 clause | |||
|
|||
|
|||
from __future__ import division, print_function | |||
|
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.
unnecessary (multiple) whiteline?
sklearn/externals/joblib/parallel.py
Outdated
@@ -5,7 +5,6 @@ | |||
# Copyright: 2010, Gael Varoquaux | |||
# License: BSD 3 clause | |||
|
|||
from __future__ import division | |||
|
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.
unnecessary whiteline?
@@ -5,7 +5,6 @@ | |||
# Author: Florian Wilhelm <[email protected]> | |||
# License: BSD 3 clause | |||
|
|||
from __future__ import division, print_function, absolute_import | |||
|
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.
unnecessary whiteline?
@adrinjalali u there ? |
@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. |
Sure !!
…On Sun, Dec 16, 2018 at 6:06 PM Adrin Jalali ***@***.***> wrote:
@surgan12 <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12791 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AfljYe1p9ZSF2xEOMA_FXWJEX4BAI0jfks5u5j5lgaJpZM4ZUshx>
.
|
@surgan12 could you please merge master? The doc build issue is fixed now there. |
So we need to resolve #12906 to make CI green here. |
sklearn/externals/_pilutil.py
Outdated
@@ -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 |
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.
Please revert all changes under sklearn/externals
: this is vendored code and it's better to keep it in its original state.
Interestingly "
That commit should have said "Merge branch 'master'" I think. Could you please try again @surgan12 ? Something along the lines of,
where upstream points to |
As far as I know, only dataset download are cached in The line you mention in the Makefile was more intended for re-running things locally, it should have no effect on CircleCI. |
@surgan12 could you do another update from the master? The issue should be solved now. |
ping @surgan12 , would you like someone else take over this one? |
@adrinjalali sorry , this just slipped out of ma head, I m on it. |
I think you may have missed these ones:
|
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.
thanks @surgan12 !
@rth I think this is finally ready to be merged. |
@adrinjalali , yep finally ready !! |
sklearn/externals/joblib/_dask.py
Outdated
@@ -1,4 +1,3 @@ | |||
from __future__ import print_function, division, absolute_import | |||
|
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.
please double check that there's no redundant blank line (e.g., blank line at the beginning of a file).
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 this is finally ready to be merged.
My comment from #12791 (comment) still needs to be addressed I think.
@rth on it !! |
sklearn/externals/joblib/_dask.py
Outdated
@@ -1,5 +1,4 @@ | |||
from __future__ import print_function, division, absolute_import | |||
|
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.
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.
thanks @surgan12 |
@qinhanmin2014 Was good experience contributing for the first time . |
This reverts commit dde62a9.
This reverts commit dde62a9.
@adrinjalali fix to #12779