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(rust, python): raise if to_datetime would have parsed input incorrectly #9675

Merged
merged 3 commits into from
Jul 2, 2023

Conversation

MarcoGorelli
Copy link
Collaborator

closes #9673

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Jul 2, 2023
@MarcoGorelli MarcoGorelli force-pushed the fix-strptime-minute-no-hour branch from 9c5d6a8 to a27bb0e Compare July 2, 2023 08:06
@MarcoGorelli MarcoGorelli force-pushed the fix-strptime-minute-no-hour branch from a27bb0e to 2246994 Compare July 2, 2023 08:20
@MarcoGorelli MarcoGorelli marked this pull request as ready for review July 2, 2023 09:23
@MarcoGorelli
Copy link
Collaborator Author

MarcoGorelli commented Jul 2, 2023

thanks for your review! merging then as the failure is unrelated

@MarcoGorelli MarcoGorelli merged commit 77c2dfd into pola-rs:main Jul 2, 2023
or not any(month in fmt for month in ("%b", "%B", "%m"))
)
and "strict conversion to datetimes failed" in str(exc)
)
Copy link

@honno honno Jul 3, 2023

Choose a reason for hiding this comment

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

FWIW I would add a reject() (after the sanity check) in situations like this, so Hypothesis will complain for us if we're generating mostly bad examples and thus not actually testing anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks - I tried that but then it was telling me that it was rejecting too many

Copy link

@honno honno Jul 3, 2023

Choose a reason for hiding this comment

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

thanks - I tried that but then it was telling me that it was rejecting too many

I would reintroduce the reject() and slap @settings(suppress_health_check=[HealthCheck.filter_too_much]) so there's a more honest recognition of that fact. I think this would also coerce Hypothesis to generate more examples, to compensate for the many filtered examples—need to check on that tho.

Ideally the strategy could be constructed in such a way its generating more valid examples. I'm pretty ignorant in this specific instance—if its to do with incompatible fmts to datetimes, maybe generate one first and then respectively generate the other?

c-peters pushed a commit to c-peters/polars that referenced this pull request Jul 14, 2023
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 rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_datetime may give incorrect results
3 participants