narwhals icon indicating copy to clipboard operation
narwhals copied to clipboard

`__getitem__` follow-ups

Open MarcoGorelli opened this issue 8 months ago • 7 comments

Keeping track of these so we don't forget them in https://github.com/narwhals-dev/narwhals/pull/2393

  • https://github.com/narwhals-dev/narwhals/pull/2393#discussion_r2052577176
  • https://github.com/narwhals-dev/narwhals/pull/2393#discussion_r2052561996
  • check about using __getitem__ with numpy scalars

MarcoGorelli avatar Apr 22 '25 20:04 MarcoGorelli

check about using __getitem__ with numpy scalars

@MarcoGorelli what was this one about?

Is it related to this commit (https://github.com/narwhals-dev/narwhals/pull/2393/commits/ff6f2eb6532e694bfa1c789b21856dd232e936e8)?

For some extra context, (#1515) unintentionally allowed np.integer to pass through to all backends - without adding any handling for them. The test the PR added only produces np.generic for pandas - whereas every other backend was just testing native python literals.

I discovered that when I attempted to remove the is_numpy_scalar part. Where I'd assumed np.integer would pass the isinstance(..., int) check like in (https://github.com/zen-xu/pyarrow-stubs/pull/221)

dangotbanned avatar Apr 23 '25 13:04 dangotbanned

From the docstring, it is not 100% clear to me whether or not __getitem__ returns a copy or not. What happens if I try to assign to the result of df[key] or df[key].to_native()?

lorentzenchr avatar Apr 25 '25 16:04 lorentzenchr

Narwhals objects are immutable, we don't support __setitem__ so it shouldn't be possible to assign there

MarcoGorelli avatar Apr 25 '25 17:04 MarcoGorelli

regarding numpy scalars, i was referring to

In [7]: df = nw.from_native(pd.DataFrame({'a': [6, 9, 8]}))

In [8]: df[np.int64(0), 'a']
Out[8]: np.int64(6)

In [9]: df['a'][np.int64(0)]
Out[9]: np.int64(6)


In [12]: df = nw.from_native(pl.DataFrame({'a': [6, 9, 8]}))

In [13]: df[np.int64(0), 'a']
Out[13]: 6

In [14]: df['a'][np.int64(0)]
Out[14]: 6

not sure if there's test coverage for this, will check

MarcoGorelli avatar Apr 25 '25 17:04 MarcoGorelli

https://github.com/narwhals-dev/narwhals/issues/2416#issuecomment-2831019969

@MarcoGorelli I'm not sure what this means 😂

If what I said in (https://github.com/narwhals-dev/narwhals/issues/2416#issuecomment-2824267311) was unclear - the change I made was intended to make sure every backend was passed an int. I assumed blindly passing a numpy scalar might not work with every backend - since we didn't have a test for it

dangotbanned avatar Apr 25 '25 17:04 dangotbanned

yup, thanks! i think calling int on a numpy scalar which we know to be of integer type is fine 👍 just checking that we have test which cover the examples from https://github.com/narwhals-dev/narwhals/issues/2416#issuecomment-2831019969 to make sure they keep working in the future

MarcoGorelli avatar Apr 25 '25 19:04 MarcoGorelli

Ah I see, yeah that makes sense @MarcoGorelli

dangotbanned avatar Apr 25 '25 20:04 dangotbanned