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

BUG: Allow unsigned integer indexing, fixes #1405 #1406

Closed
wants to merge 1 commit into from

Conversation

gerritholl
Copy link
Contributor

@gerritholl gerritholl commented May 11, 2017

Permit indexing with unsigned integers. This should fix #1405.

Permit indexing with unsigned integers.  This should fix pydata#1405.
@shoyer
Copy link
Member

shoyer commented May 11, 2017

Indeed, looks like this should do the trick, but we definitely need a regression test before merging this (also a bugfix note, see the checklist in your first comment).

@gerritholl
Copy link
Contributor Author

Perhaps I was too fast, I edited it directly in github.

@fmaussion
Copy link
Member

@gerritholl when files are edited on github it creates a branch on your fork (here gerritholl:patch-3) which you can clone locally and update with the usual workflow.

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

@gerritholl - thanks for the fix here. Do you think you'll come back around to finish this PR up? Otherwise, I'll mark as "Contributions Welcome".

@@ -58,7 +58,7 @@ def canonicalize(indexer):
'1d arrays')
if indexer.dtype.kind == 'b':
indexer, = np.nonzero(indexer)
elif indexer.dtype.kind != 'i':
elif indexer.dtype.kind not in 'ui':
Copy link
Member

Choose a reason for hiding this comment

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

nit: this feels too cute. I suggest writing this as:

elif indexer.dtype.kind not in ['u', 'i']:

@gerritholl
Copy link
Contributor Author

I may make time for it later

@jhamman
Copy link
Member

jhamman commented Sep 1, 2017

I have an updated version of this PR up at #1544

@jhamman
Copy link
Member

jhamman commented Sep 1, 2017

I'm closing in favor of #1473

@jhamman jhamman closed this Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using uint64 for Dataset indexing gives ValueError
4 participants