[MRG+1] Astype fix #4645

TomDLT · 2015-04-29T17:32:23Z

I extended check_array to add support for accept_sparse=True
and solve the 3 errors by changing astype into check_array when sparse matrix are possible

I will check the other uses of astype tomorrow.

TODO:

fix the test error
finish changing astype to check_array, when we may have a sparse matrix
clean some remaining usage of warn_if_not_float to use check_array instead

agramfort · 2015-04-29T19:43:20Z

thanks @TomDLT

Travis is not happy. You have failing tests.

TomDLT · 2015-04-30T09:43:02Z

OK maybe I will start testing before pushing

ogrisel · 2015-04-30T09:59:32Z

Also it would be great to rebase this branch on top of current master while removing the upstream/master merge commit by @jrings.

agramfort · 2015-04-30T10:00:55Z

OK maybe I will start testing before pushing

Sounds like a plan ;)

amueller · 2015-04-30T15:19:19Z

meh, you can also just check travis after pushing ;) but that utually takes longer.

TomDLT · 2015-04-30T18:19:48Z

check_array is now enhanced to accept :
-accept_sparse=True to accept all sparse matrix formats
-dtype with an array of accepted types
-warn_on_dtype=True to warn if the dtype as been changed

Big thanks to @ogrisel that worked on this with me

TODO:
-fix the error
-finish changing astypeto check_array, when we may have a sparse matrix
-clean some warn_if_not_float using check_array

I will look at this monday

ogrisel · 2015-05-04T09:20:22Z

To fix the test that fail on travis, you might want to setup a conda env with the same versions for numpy and scipy.

conda create -n oldnumpy python=2.6 numpy=1.6.2 scipy=0.11.0
source activate oldnumpy
make clean test

ogrisel · 2015-05-04T11:36:07Z

Good catch, travis is now green. I re-wrapped and moved your todo list to put it in the description of the PR. Once all the boxes are ticked please edit the title of the PR to replace [WIP] (work in progress) to [MRG] (ready for merge reviews).

TomDLT · 2015-05-04T11:38:10Z

In k_means.py, do we want to change _check_test_data to use the enhanced version of check_array?
It will change a RuntimeWarning into a DataConversionWarning

note : warn_if_not_float is not used anymore

amueller · 2015-05-04T13:47:08Z

I'm not sure how great an idea of warn_if_no_float was (I think it was mine). We should probably just convert to float.

ogrisel · 2015-05-04T13:58:26Z

I'm not sure how great an idea of warn_if_no_float was (I think it was mine). We should probably just convert to float.

This is what we are doing in this PR when you pass check_array(X, dtype=[np.float64, np.float32, np.float16]) which can also be written check_array(X, dtype=FLOAT_DTYPES) for convenience.

ogrisel · 2015-05-04T14:01:19Z

In k_means.py, do we want to change ˋ_check_test_dataˋ to use the enhanced version of check_array? It will change a ˋRuntimeWarningˋ into a ˋDataConversionWarningˋ

+1 for using check_array(X, dtype=FLOAT_DTYPES) in that case as well. I don't thing that changing the type of warning is such a big deal. Increasing the consistency of the code base is good.

ogrisel · 2015-05-04T14:05:30Z

sklearn/datasets/twenty_newsgroups.py

-    X_train = X_train.astype(np.float64)
-    X_test = X_test.astype(np.float64)
+    X_train = astype(X_train, np.float64, copy=False)
+    X_test = astype(X_test, np.float64, copy=False)


There is no need to set copy=False for this use case as the original data type is int16 for sure as explained in the previous commit. There is no way to avoid the malloc / dtype conversion in that case. We can keep:

X_train = X_train.astype(np.float64) X_test = X_test.astype(np.float64)

TomDLT · 2015-05-04T15:58:03Z

Is there a clean way to revert some old commits without a total rebase?

amueller · 2015-05-06T15:49:38Z

Run a git grep. If it is not used (and I think it shouldn't be), please remove it.

amueller · 2015-05-06T15:51:07Z

sklearn/utils/validation.py

+        # not a data type (e.g. a column named dtype in a pandas DataFrame)
+        dtype_orig = None
+
+    if dtype_numeric:


I'm not 100% sure if we want to keep this behavior. In what cases do we want to pass through integer data? Or bytes? But maybe that is for another PR? I'm not sure....
We could also just set dtype = NUMERIC_DTYPES (which would be a long list) to remove this branch at least.

I also thought about that option but I think it's fine to leave it this way for now.

amueller · 2015-05-06T15:53:51Z

This is awesome, thanks!
I am a bit uncertain about the conversion in spectral_embedding, otherwise this LGTM.

I'm not sure how good the "numeric" option was, and maybe we should remove it now, and use explicit dtypes instead? But I think we can merge this before that refactoring.

TomDLT · 2015-05-07T14:54:07Z

Do you want me to squash all commits in only one?

ogrisel · 2015-05-07T16:34:19Z

Maybe we should deprecate warn_if_not_float instead of removing it. I know those utilities are not supposed to be part of the public API but... Any opinion @amueller ?

ogrisel · 2015-05-07T16:35:06Z

I'm not sure how good the "numeric" option was, and maybe we should remove it now, and use explicit dtypes instead? But I think we can merge this before that refactoring.

+1 for thinking about this point again once this PR has been merged.

ogrisel · 2015-05-07T16:37:26Z

Do you want me to squash all commits in only one?

I think so yes.

amueller · 2015-05-07T17:25:54Z

I think removing warn_if_not_float is fine. And we should really split public / private utils :-/

amueller · 2015-05-07T17:26:25Z

+1 on squashing everything and +1 on merge.

amueller · 2015-05-07T17:31:29Z

See #4685 for tracking the possible removal of "numeric".

TomDLT · 2015-05-07T17:32:49Z

Squashed

TomDLT · 2015-05-07T17:35:57Z

Just curiosity, to deprecate warn_if_not_float, how can I do a relative import of deprecated ?
(inside utils/validation.py, import from utils/__init__.py)

amueller · 2015-05-07T17:37:07Z

from . import deprecated maybe?

TomDLT · 2015-05-07T17:57:51Z

It did not work for me

amueller · 2015-05-09T21:57:34Z

@ogrisel merge?

ogrisel · 2015-05-19T13:59:17Z

Fine with the removal of warn_if_not_float then. +1 for merge.

Can you please rebase on top of the current master once again? There is a small conflict in sklearn/preprocessing/data.py (easy to resolve, just keep the min_max_axis import). All tests pass on my box but I would like travis to run once again on this PR before merging.

ENH improve check_array to warn on dtype conversions ENH make check_array accept several dtypes ENH change validation with improved check_array ENH change astype to avoid copy if possible ENH remove warn_if_not_float

TomDLT · 2015-05-19T14:32:44Z

Rebased on top of master

[MRG+1] Astype fix

amueller · 2015-05-19T14:54:26Z

Thanks :)

amueller · 2015-05-19T14:55:02Z

This is really great btw 🍻

TomDLT · 2015-05-19T15:18:09Z

Thanks :)

agramfort · 2015-05-19T15:31:00Z

🍻 !

ogrisel · 2015-05-21T11:28:13Z

I forgot to drink a beer on that PR: 🍻

agramfort mentioned this pull request Apr 29, 2015

PERF: Prevent using copy in astype if not needed #4575

Closed

TomDLT force-pushed the astype_fix branch 2 times, most recently from 0222c8b to 411b868 Compare April 30, 2015 17:43

TomDLT force-pushed the astype_fix branch from 9dc64a3 to c97c12b Compare May 4, 2015 09:28

ogrisel changed the title ~~Astype fix~~ [WIP] Astype fix May 4, 2015

TomDLT changed the title ~~[WIP] Astype fix~~ [MRG] Astype fix May 4, 2015

ogrisel reviewed May 4, 2015
View reviewed changes

TomDLT closed this May 4, 2015

amueller reviewed May 6, 2015
View reviewed changes

amueller mentioned this pull request May 7, 2015

Revisit dtype="numeric" #4685

Closed

TomDLT force-pushed the astype_fix branch from 1d861b9 to d4f8b81 Compare May 7, 2015 17:31

ENH improve check_array

89c1018

ENH improve check_array to warn on dtype conversions ENH make check_array accept several dtypes ENH change validation with improved check_array ENH change astype to avoid copy if possible ENH remove warn_if_not_float

TomDLT force-pushed the astype_fix branch from d4f8b81 to 89c1018 Compare May 19, 2015 14:14

amueller added a commit that referenced this pull request May 19, 2015

Merge pull request #4645 from TomDLT/astype_fix

9385c45

[MRG+1] Astype fix

amueller merged commit 9385c45 into scikit-learn:master May 19, 2015

This was referenced May 19, 2015

Audit our usage of numpy.astype to remove unecessary memory copies #4573

Closed

[MRG+2] use astype to avoid unnecessary copy #4754

Merged

[MRG+1] Astype fix #4645

[MRG+1] Astype fix #4645

Conversation

TomDLT commented Apr 29, 2015

agramfort commented Apr 29, 2015

TomDLT commented Apr 30, 2015

ogrisel commented Apr 30, 2015

agramfort commented Apr 30, 2015

amueller commented Apr 30, 2015

TomDLT commented Apr 30, 2015

ogrisel commented May 4, 2015

ogrisel commented May 4, 2015

TomDLT commented May 4, 2015

amueller commented May 4, 2015

ogrisel commented May 4, 2015

ogrisel commented May 4, 2015

ogrisel May 4, 2015

Choose a reason for hiding this comment

TomDLT commented May 4, 2015

amueller commented May 6, 2015

amueller May 6, 2015

Choose a reason for hiding this comment

ogrisel May 7, 2015

Choose a reason for hiding this comment

amueller commented May 6, 2015

TomDLT commented May 7, 2015

ogrisel commented May 7, 2015

ogrisel commented May 7, 2015

ogrisel commented May 7, 2015

amueller commented May 7, 2015

amueller commented May 7, 2015

amueller commented May 7, 2015

TomDLT commented May 7, 2015

TomDLT commented May 7, 2015

amueller commented May 7, 2015

TomDLT commented May 7, 2015

amueller commented May 9, 2015

ogrisel commented May 19, 2015

TomDLT commented May 19, 2015

amueller commented May 19, 2015

amueller commented May 19, 2015

TomDLT commented May 19, 2015

agramfort commented May 19, 2015 via email

ogrisel commented May 21, 2015