-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
FIX safe indexing for polars Series
#28521
FIX safe indexing for polars Series
#28521
Conversation
Series
Did you fixed the ones you found? Or are there more in addition to what is in this PR? A general though I had after reading the diff: should we move to using |
Sorry that my wording is not accurate. The fixes I made are simply to pass the
Yes I would prefer explicit naming as well, and since this is not public API changing the names should not cause problems. But somehow "pandas" and "dataframe" both means
If we decide to make namings specific I think we should do it in another PR and then come back to this one. After all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of suggestion. In general, it looks good. I think this is better to add a couple of comments because there is some implicit knowledge in the branching.
"series", "index", "slice", "sparse_csr", "sparse_csc"} | ||
"series", "index", "slice", "sparse_csr", "sparse_csc", \ | ||
"sparse_csr_array", "sparse_csc_array", "pyarrow", "polars", \ | ||
"polars_series"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this function start to not be maintainable. We will need to modify the signature and split contructor_name
into different parameter such as container_type
, constuctor_lib
, and something to reflect dense vs. sparse.
So here, let's keep the code that you suggest but we need to follow-up on this code.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
I think we must deliberately decide which version of polars we support. Depending on that different methods/syntax are used for indexing, see, e.g., pola-rs/polars#4924. |
It seems that |
Not at all. Over the past year, a lot of deprecations happened. |
I think that we defined for the moment a mean dependency of 0.19.12 and we are testing for it as well as the latest one available. So if there is a change of behaviour we should catch it either via a deprecation warning or a failure (if polars does not warn first). I assume that regarding the version, we will have to be flexible to bump easily the minimum version since polars is releasing fast for the moment. |
If polars really break something in the indexing, I assume that we have to decide either to:
@lorentzenchr do you envisage something different? |
I tested locally that min and latest versions both work. Just out of curiosity in which job(s) do we test against the minimum versions of those libraries? |
The only place that we test for the minimum pandas or polars are only in the |
Do we also test latest polars and pandas? |
Emm but that one does not seem to run the test suite?
For instance pylatest_conda_forge_mkl_linux-64 I think. |
Indeed, it will be some indirect testing through examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think that we can first merge this as-is and reconsider the way we build our matrix for the CI in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I’ll merge under the assumption of follow-up PRs.
Towards #28488.
The initial goal of this PR is to make
_safe_indexing
work for polars Series and changing_is_polars_df
into_is_polars_df_or_series
suffices.However when extending the tests for pandas Series and DataFrame to polars, I found some other places that may need to be fixed (e.g.,
_polars_indexing
).