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: IntervalIndex.get_indexer raising for read only array #53703

Merged
merged 3 commits into from Jun 19, 2023

Conversation

phofl
Copy link
Member

@phofl phofl commented Jun 16, 2023

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@phofl phofl added the Interval Interval data type label Jun 16, 2023
@phofl phofl requested a review from WillAyd as a code owner June 16, 2023 20:29
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm

@WillAyd
Copy link
Member

WillAyd commented Jun 16, 2023

Should we just generally go back to using ndarray instead of memoryviews? If we set that expectation could be a good change for new contributors to pick up

@phofl
Copy link
Member Author

phofl commented Jun 16, 2023

This change is only necessary because cython does not support const with fused types unfortunately

@WillAyd
Copy link
Member

WillAyd commented Jun 16, 2023

Ah OK. So I guess the pattern is for fused types prefer ndarray (unless that gets fixed/implemented) elsewhere memview?

@phofl
Copy link
Member Author

phofl commented Jun 16, 2023

Yep that makes sense I guess


result = idx.get_indexer_non_unique(arr)[0]
expected = np.array([0, 1])
tm.assert_numpy_array_equal(result, expected, check_dtype=False)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm what's going on with the dtype? Wouldn't expect that issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Windows/non windows stuff and since the dtype really doesnt matter here, I just ignored it

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can explicitly specify the type of the np.array call instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would kill the 32 bit build... Open for suggestions but dtype really doesn't matter

@jorisvandenbossche
Copy link
Member

So I guess the pattern is for fused types prefer ndarray (unless that gets fixed/implemented) elsewhere memview?

And that's also what we already do in most places. For example in groupby.pyx, you will see that we typically use ndarray for the input values (which use a fused type and can be const), while mostly use memoryviews for output values (which are writeable) or labels/mask (which don't use fused types).
(although it's certainly not exact, I see some cases using ndarray that could use memoryview)

@phofl
Copy link
Member Author

phofl commented Jun 19, 2023

merging this, happy to address comments in a follow up.

@phofl phofl merged commit 25c579a into pandas-dev:main Jun 19, 2023
30 of 34 checks passed
@phofl phofl added this to the 2.1 milestone Jun 19, 2023
@phofl phofl deleted the interval_read_only branch June 19, 2023 20:07
canthonyscott pushed a commit to canthonyscott/pandas-anthony that referenced this pull request Jun 23, 2023
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interval Interval data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants