Skip to content

FIX Explicit int cast in NN binary trees #18654

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
Oct 31, 2020
Merged

FIX Explicit int cast in NN binary trees #18654

merged 2 commits into from
Oct 31, 2020

Conversation

larsoner
Copy link
Contributor

Using latest NumPy and scikit-learn master and Cython , I get:

  File "/home/larsoner/python/mne-python/mne/surface.py", line 522, in __init__
    self.query = partial(_safe_query, func=BallTree(xhs).query,
  File "sklearn/neighbors/_binary_tree.pxi", line 1077, in sklearn.neighbors._ball_tree.BinaryTree.__init__
TypeError: 'numpy.float64' object cannot be interpreted as an integer

I assume this has to do with more strict type checking somewhere, so this just uses an explicit cast.

I'm not 100% sure this is the right fix, but things at least seem to work with it in place.

@alfaro96
Copy link
Member

alfaro96 commented Oct 20, 2020 •
edited
Loading

This issue is being investigated in #18644 (I think that it is related with the downcasting for NumPy data types).

@larsoner
Copy link
Contributor Author

Sounds good, feel free to close if this isn't the preferred solution (though it at least makes some sense since I think this is what was implicitly happening before anyway?)

@alfaro96
Copy link
Member

alfaro96 commented Oct 20, 2020 •
edited
Loading

That is. An implicit conversion from float division to integer was happening before. However, I kind of prefer a generic solution for this problem because it is happening in other Cython extension modules (e.g., sklearn.utils._seq_dataset).

WDYT @thomasjpfan?

@thomasjpfan
Copy link
Member

@alfaro96 I am seeing this in multiple issues. I think we need to start being more careful and do explicit casting from now on.

@alfaro96
Copy link
Member

alfaro96 commented Oct 30, 2020 •
edited
Loading

Then, let us merge this PR. I will submit another PR applying explicit casting to the Cython extension modules causing the failure in the travis CRON job.

alfaro96
alfaro96 approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@alfaro96 alfaro96 left a comment

Choose a reason for hiding this comment

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

LGTM @larsoner.

@github-actions github-actions bot removed the ci sync label Oct 30, 2020
thomasjpfan
thomasjpfan approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@rth
Copy link
Member

rth commented Oct 31, 2020

Thanks @larsoner !

@rth rth changed the title BUG: Explicit int cast FIX Explicit int cast in NN binary trees Oct 31, 2020
@rth rth merged commit fb67c7b into scikit-learn:master Oct 31, 2020
@ogrisel ogrisel mentioned this pull request Nov 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@alfaro96 alfaro96 alfaro96 approved these changes

Assignees
No one assigned
Labels
module:neighbors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@larsoner @alfaro96 @thomasjpfan @rth