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

fix(python): Raise properly for slices not supported by LazyFrame #15331

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

petrosbar
Copy link
Contributor

Found a couple more slice cases that are mishandled by LazyFrame. Specifically, both cases below are not supported by LazyFrame but they currently raise incorrect exceptions/errors:

  1. slice(None, 0, k), where k < 0
import polars as pl

ldf = pl.LazyFrame({"a": [1, 2, 3]})
ldf[:0:-1].collect()
# TypeError: '>=' not supported between instances of 'int' and 'NoneType'
  1. slice(i, None, k), where i, k < 0
import polars as pl

ldf = pl.LazyFrame({"a": [1, 2, 3]})
ldf[-1::-1].collect()
# OverflowError: can't convert negative int to unsigned

Both these cases should raise a ValueError: the given slice is not supported by lazy computation

@github-actions github-actions bot added fix Bug fix python Related to Python Polars labels Mar 27, 2024
@alexander-beedie alexander-beedie self-assigned this Mar 27, 2024
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Mar 27, 2024

Interesting! I need to check why the parametric tests didn't catch some of these; I think the fixes looks good, but I want to update the parametric tests such that they find these, to make sure nothing else slips through 🤔

@petrosbar
Copy link
Contributor Author

@alexander-beedie Actually, I did find these cases (and also #15297) using Hypothesis. I just had to actually modify the strategy a bit.

I think the problem is here:
https://github.com/pola-rs/polars/blob/main/py-polars/polars/testing/parametric/primitives.py#L426

Probably because of the way Hypothesis works, this isn't really very "random", and it is one of the reasons why upgrading Hypothesis gives warnings and #14798 cannot be merged.

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Mar 27, 2024

it is one of the reasons why upgrading Hypothesis gives warnings and #14798 cannot be merged.

Adjusting the weights of the test (null probability) made it work better - it found both errors. I have a TODO to fix use of random though; this is more incentive to get that done 😅

@petrosbar
Copy link
Contributor Author

it is one of the reasons why upgrading Hypothesis gives warnings and #14798 cannot be merged.

Adjusting the weights of the test (null probability) made it work better - it found both errors. I have a TODO to fix use of random though; this is more incentive to get that done 😅

Yes. But as you said that wouldn't solve the actual problem either.
Can you see this branch? main...petrosbar:polars:parametric
This would expose the faulty cases.

@petrosbar
Copy link
Contributor Author

Note that in this branch, the changed probabilities in test_strategy_null_probability are a bit irrelevant. I had to adjust them, otherwise the test case was flaky. That also shows that the check
if random.random() < null_probability: does not work as expected.

@alexander-beedie
Copy link
Collaborator

Note that in this branch, the changed probabilities in test_strategy_null_probability are a bit irrelevant. I had to adjust them, otherwise the test case was flaky. That also shows that the check if random.random() < null_probability: does not work as expected.

Indeed; I need to run around the parametric code addressing usage of the standard random.
Anyway, the fixes here look good 😎

@alexander-beedie alexander-beedie merged commit 39b486e into pola-rs:main Mar 27, 2024
12 checks passed
@petrosbar petrosbar deleted the slice-raise-proper branch March 27, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants