8000 [MRG] Fix numpy FutureWarning by naoyak · Pull Request #11704 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Fix numpy FutureWarning #11704

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.

Sign up for GitHub

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
Merged
merged 2 commits into from
Jul 30, 2018
Merged

[MRG] Fix numpy FutureWarning #11704

merged 2 commits into from
Jul 30, 2018

Conversation

naoyak
Copy link
Contributor
@naoyak naoyak commented Jul 29, 2018 •
edited
Loading

Reference Issues/PRs

Part of #11697, continue #11701

What does this implement/fix? Explain your changes.

Attempt to get rid of FutureWarnings by accessing ndarrays with tuples

Any other comments?

@naoyak naoyak force-pushed the fix-numpy-futurewarning branch from 796017d to e8f4388 Compare July 29, 2018 20:16
@naoyak naoyak mentioned this pull request Jul 29, 2018
Closed
jnothman
jnothman reviewed Jul 29, 2018
View reviewed changes
sklearn/cluster/birch.py Outdated
@@ -74,7 +74,7 @@ def _split_node(node, threshold, branching_factor):

farthest_idx = np.unravel_index(
dist.argmax(), (n_clusters, n_clusters))
node1_dist, node2_dist = dist[[farthest_idx]]
node1_dist, node2_dist = dist[tuple(farthest_idx)]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is the intention here. I think the list might have been intended to extract a result with a different shape.... Please double check

@naoyak naoyak force-pushed the fix-numpy-futurewarning branch from 3fbc7e7 to a2ee963 Compare July 30, 2018 00:15
@naoyak
Copy link
Contributor Author
naoyak commented Jul 30, 2018

@jnothman thanks for the input

jnothman
jnothman reviewed Jul 30, 2018
View reviewed changes
sklearn/cluster/birch.py Outdated
@@ -74,7 +74,7 @@ def _split_node(node, threshold, branching_factor):

farthest_idx = np.unravel_index(
dist.argmax(), (n_clusters, n_clusters))
node1_dist, node2_dist = dist[[farthest_idx]]
node1_dist, node2_dist = dist[tuple([farthest_idx])]
Copy link
Member

Choose a reason for hiding this comment

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

I'm still confused about whether this is doing the same as it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The list was wrapping farthest_idx (a tuple) inside a list, now it's a tuple inside a tuple - I'm a bit confused too even though it's passing.

Copy link
Member

Choose a reason for hiding this comment

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

I've checked the code, and this does look right (semantically). But the standard way to indicate a tuple of one element is: (farthest_idx,).

@naoyak naoyak force-pushed the fix-numpy-futurewarning branch from 9449f49 to a2ee963 Compare July 30, 2018 01:06
jnothman
jnothman reviewed Jul 30, 2018
View reviewed changes
Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

sklearn/cluster/birch.py Outdated
@@ -74,7 +74,7 @@ def _split_node(node, threshold, branching_factor):

farthest_idx = np.unravel_index(
dist.argmax(), (n_clusters, n_clusters))
node1_dist, node2_dist = dist[[farthest_idx]]
node1_dist, node2_dist = dist[tuple([farthest_idx])]
Copy link
Member

Choose a reason for hiding this comment

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

I've checked the code, and this does look right (semantically). But the standard way to indicate a tuple of one element is: (farthest_idx,).

@naoyak
Copy link
Contributor Author
naoyak commented Jul 30, 2018

Sounds good, changed the tuple notation.

Otherwise I think we need SciPy nightlies on 3.7 to make the other FutureWarnings go away.

@jnothman jnothman merged commit 774ae89 into scikit-learn:master Jul 30, 2018
@jnothman
Copy link
Member

Thanks

numpy.percentile is a superior implementation so I wouldn't mind seeing scoreatpercentile disappear anyway.

Do you want to look into the Travis install script that's giving us to wrong scipy?

@naoyak
Copy link
Contributor Author
naoyak commented Jul 30, 2018 •
edited
Loading

Do you want to look into the Travis install script that's giving us to wrong scipy?

@jnothman we'd need to revert to Python 3.6 or wait for SciPy nightlies to build on Python 3.7. I opened a PR here: MacPython/scipy-wheels#33

@jnothman
Copy link
Member
jnothman commented Jul 30, 2018 via email

@naoyak naoyak deleted the fix-numpy-futurewarning branch July 30, 2018 09:23
@naoyak naoyak mentioned this pull request Jul 31, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@naoyak @jnothman
0