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

Jump to bottom

[MRG] BUG ensure object array are properly casted when dtype=object #16076

Merged
merged 32 commits into from Jan 15, 2020
Merged

[MRG] BUG ensure object array are properly casted when dtype=object #16076

merged 32 commits into from Jan 15, 2020

Conversation

@alexshacked
Copy link
Contributor

@alexshacked alexshacked commented Jan 9, 2020 •
edited by glemaitre
Loading

closes #16036

Fix a bug where calling np.array(..., dtype=object) will create a N-D array while algorithms are expecting a 1-D array with objects inside (similar to a list).
thomasjpfan
thomasjpfan reviewed Jan 9, 2020
View changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Thank you for the PR @alexshacked !

Loading

sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
Loading
@glemaitre glemaitre changed the title [MRG] Using dbscan with precomputed neighbors gives an error in 0.22.… [MRG] BUG ensure object array are properly casted when dtype=object Jan 9, 2020
glemaitre
glemaitre requested changes Jan 9, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

Looks good. A couple of changes.

Please add an entry to the change log at doc/whats_new/v0.20.rst under bug fixes. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

Loading

sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved
Loading
jnothman
jnothman reviewed Jan 9, 2020
View changes
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 9, 2020

Loading

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jan 9, 2020

Oh I see, I was seeking for np.array(..., dtype=object).
Then, I agree to move it either as it is now (I would rename it _to_object_array) or even in utils if we have something similar in other file. I will look at it.

Loading

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jan 9, 2020

So we need to call _to_object_array for sklearn.neighbors._base: l.945-948; l.952-953

NB: I searched for the patter [:] = and filter that it was preceded by the creation of a numpy object array.

Loading

glemaitre
glemaitre reviewed Jan 9, 2020
View changes
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 9, 2020

So we need to call _to_object_array for sklearn.neighbors._base: l.945-948; l.952-953

I see similar here:

sklearn/preprocessing/tests/test_label.py=435=def test_multilabel_binarizer_non_integer_labels():
sklearn/preprocessing/tests/test_label.py:436:    tuple_classes = np.empty(3, dtype=object)
sklearn/preprocessing/tests/test_label.py-437-    tuple_classes[:] = [(1,), (2,), (3,)]
--
sklearn/neighbors/_classification.py:541:            pred_labels = np.zeros(len(neigh_ind), dtype=object)
sklearn/neighbors/_classification.py-542-            pred_labels[:] = [_y[ind, k] for ind in neigh_ind]

but otherwise agree it's all in radius_neighbors

Loading

@alexshacked
Copy link
Contributor Author

@alexshacked alexshacked commented Jan 9, 2020

@glemaitre change log is in v0.20.rst? I thought v0.23.rst

Loading

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jan 9, 2020

v0.23.rst

Loading

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jan 9, 2020

Ups my automatic answering is broken :)

Loading

@alexshacked
Copy link
Contributor Author

@alexshacked alexshacked commented Jan 9, 2020

ok. v0.23 then. Thanks @glemaitre

Loading

alexshacked and others added 11 commits Jan 9, 2020
@alexshacked
[MRG] Using dbscan with precomputed neighbours. (scikit-learn#16036) …
887f154 
…change log
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
e582a40 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
eda4787 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/tests/test_neighbors.py
eedad66 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/tests/test_neighbors.py
bfeee92 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
e13297c 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
a512168 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
a538fac 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
19b23b2 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
af6b74d 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked
(scikit-learn#16036) refactoring with new function _to_object_array()
e2d9f8b
glemaitre
glemaitre reviewed Jan 10, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment •
edited
Loading

We will need to apply the _to_object_array function on the following line:

sklearn/preprocessing/tests/test_label.py=435=def test_multilabel_binarizer_non_integer_labels():
sklearn/preprocessing/tests/test_label.py:436:    tuple_classes = np.empty(3, dtype=object)
sklearn/preprocessing/tests/test_label.py-437-    tuple_classes[:] = [(1,), (2,), (3,)]

I propose to add a docstring (as you did earlier) and move the _to_object_array function in sklearn/utils/__init__.py. Then, we can import it in neighbors and preprocessing.

We just need to add a small test in sklearn/utils/tests/test_utils.py to check the expected behavior:

@pytest.mark.parametrize(
    "sequence",
    [[np.array(1), np.array(2)], [[1, 2], [3, 4]]]
)
test_to_object_array(sequence):
    out = _to_object_array(sequence)
    assert isinstance(out, ndarray)
    assert out.dtype.kind == 'O'
    assert out.ndim == 1

Loading

doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved
Loading
doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved
Loading
doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
alexshacked and others added 2 commits Jan 10, 2020
@alexshacked @glemaitre
Update doc/whats_new/v0.23.rst
93d87d5 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update doc/whats_new/v0.23.rst
71f9c7f 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
alexshacked and others added 3 commits Jan 10, 2020
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
33b68e5 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked
[MRG] Using dbscan with precomputed neighbours. (scikit-learn#16036) …
5edfeb6 
…moved to_object_array() to sklearn.utils
@alexshacked
[MRG] Using dbscan with precomputed neighbours. (scikit-learn#16036)…
d3e05dd 
… fix PEP8 errors
@alexshacked
Copy link
Contributor Author

@alexshacked alexshacked commented Jan 10, 2020

Hi @glemaitre. Moved function to_object_array() to sklearn.utils and changed the message in the change log of v0.23.rst

Loading

glemaitre
glemaitre approved these changes Jan 10, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

Apart of making the function private LGTM. @alexshacked you can accept my suggestion and this would be enough.

Loading

sklearn/utils/__init__.py Outdated Show resolved Hide resolved
Loading
sklearn/preprocessing/tests/test_label.py Outdated Show resolved Hide resolved
Loading
sklearn/utils/__init__.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
sklearn/neighbors/_base.py Outdated Show resolved Hide resolved
Loading
@alexshacked
Copy link
Contributor Author

@alexshacked alexshacked commented Jan 10, 2020 •
edited
Loading

Sorry about this @glemaitre . I thought one underscore means private inside the class, not private inside the package. Will restore the underscore

Loading

alexshacked and others added 11 commits Jan 10, 2020
@alexshacked @glemaitre
Update sklearn/preprocessing/tests/test_label.py
a992048 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/utils/__init__.py
ece10a1 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
6a75af4 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
83df8c6 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
3508571 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
8ddd3e5 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
c302eb9 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked @glemaitre
Update sklearn/neighbors/_base.py
cf8ada9 
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@alexshacked
[MRG] Using dbscan with precomputed neighbours. (scikit-learn#16036) …
8df3f1c 
…removed unnecessary comment
@alexshacked
fixed import statement
aaf778e
@alexshacked
[MRG] Using dbscan with precomputed neighbours. (scikit-learn#16036) …
e5dcb28 
…restored underscore
@glemaitre glemaitre added this to the 0.22.2 milestone Jan 13, 2020
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jan 13, 2020

LGTM. @jnothman @thomasjpfan Could you have a look. I added the regression tag and tag it as a candidate for 0.22.2

Loading

TomDLT
TomDLT approved these changes Jan 13, 2020
View changes
Copy link
Member

@TomDLT TomDLT left a comment

LGTM

Loading

sklearn/utils/__init__.py Show resolved Hide resolved
Loading
doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved
Loading
sklearn/utils/__init__.py Outdated Show resolved Hide resolved
Loading
@alexshacked
Copy link
Contributor Author

@alexshacked alexshacked commented Jan 13, 2020

Thanks for your comments @TomDLT. Will apply them all.

Loading

@TomDLT TomDLT merged commit c4ea377 into scikit-learn:master Jan 15, 2020
11 of 17 checks passed
Loading
@TomDLT
Copy link
Member

@TomDLT TomDLT commented Jan 15, 2020

Thanks @alexshacked !

Loading

thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this issue Feb 22, 2020
@alexshacked @thomasjpfan
FIX ensure object array are properly casted when dtype=object (scikit…
0791946 
…-learn#16076)
jeremiedbb added a commit to jeremiedbb/scikit-learn that referenced this issue Feb 28, 2020
@alexshacked @jeremiedbb
FIX ensure object array are properly casted when dtype=object (scikit…
9c85f44 
…-learn#16076)
ogrisel added a commit that referenced this issue Feb 28, 2020
@alexshacked @jeremiedbb
@oleksandr-pavlyk @ogrisel @glemaitre @jnothman @thomasjpfan
[WIP] 0.22.2 release branch (#16587)
4b7331e 
* FIX ensure object array are properly casted when dtype=object (#16076)

* DOC Docstring example of classifier should import classifier (#16430)

* MNT Update nightly build URL and release staging config (#16435)

* BUG ensure that estimator_name is properly stored in the ROC display (#16500)

* BUG ensure that name is properly stored in the precision/recall display (#16505)

* ENH Perform KNN imputation without O(n^2) memory cost (#16397)

* bump scikit-learn version for binder

* bump version to 0.22.2

* MNT Skips failing SpectralCoclustering doctest (#16232)

* TST Updates test for deprecation in pandas.SparseArray (#16040)

* move 0.22.2 what's new entries (#16586)

* add 0.22.2 in the news of the web site frontpage

* skip test_ard_accuracy_on_easy_problem

Co-authored-by: alexshacked <al.shacked@gmail.com>
Co-authored-by: Oleksandr Pavlyk <oleksandr-pavlyk@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Joel Nothman <joel.nothman@gmail.com>
Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this issue Mar 3, 2020
@alexshacked @panpiort
FIX ensure object array are properly casted when dtype=object (scikit…
7afde81 
…-learn#16076)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman

@thomasjpfan thomasjpfan

@glemaitre glemaitre

@TomDLT TomDLT

Assignees
No one assigned
Labels
Regression
Projects
None yet
Milestone
0.22.2
Linked issues

Successfully merging this pull request may close these issues.

Using dbscan with precomputed neighbors gives an error in 0.22.X, but not in 0.21.3.
5 participants
@alexshacked @jnothman @glemaitre @TomDLT @thomasjpfan