ENH add narwhals as dependency #31127
Conversation
| if hasattr(X, "iloc"): | ||
| # TODO: we should probably use _is_pandas_df_or_series(X) instead but this | ||
| # would require updating some tests such as test_train_test_split_mock_pandas. | ||
| # TODO: Should also work with _narwhals_indexing, but | ||
| # test_safe_indexing_pandas_no_settingwithcopy_warning | ||
| # does not pass. | ||
| return _pandas_indexing(X, indices, indices_dtype, axis=axis) | ||
| elif _is_polars_df_or_series(X): | ||
| return _polars_indexing(X, indices, indices_dtype, axis=axis) | ||
| elif nw.dependencies.is_into_dataframe(X) or nw.dependencies.is_into_series(X): | ||
| return _narwhals_indexing(X, indices, indices_dtype, axis=axis) |
There was a problem hiding this comment.
This whole section should read
if nw.dependencies.is_into_dataframe(X) or nw.dependencies.is_into_series(X):
return _narwhals_indexing(X, indices, indices_dtype, axis=axis)and the pandas/iloc path should be removed. But currently, it does not pass the test test_safe_indexing_pandas_no_settingwithcopy_warning.
@MarcoGorelli You might have additional insights.
There was a problem hiding this comment.
thanks for the ping, i'll take a look next week!
There was a problem hiding this comment.
To be honest I don't understand this test. It checks that pandas makes a copy, but only when indexing with [0, 1]. On the main branch, changing that to slice(0, 2) is enough to make the test fail
I'll open a separate issue to discuss
Narwhals objects (DataFrame, Series) are immutable so this isn't something you should need to worry about with Narwhals anyway
There was a problem hiding this comment.
Here you go: #31290
Anyone in particular we should tag in it?
Reference Issues/PRs
Closes #31049.
What does this implement/fix? Explain your changes.
This PR adds narwhals as runtime dependency like joblib and uses narwhals in:
_safe_indexingAny other comments?
Not yet.