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

perf: Reorder conditions in is_leap_year #19602

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Conversation

janpipek
Copy link
Contributor

@janpipek janpipek commented Nov 2, 2024

This is a completely random shot as I was just reading polars source code for fun and have no insights into it. I can imagine the rust compiler optimises this anyway but I'd rule out 75 % of cases in the first check rather than 0.25 %.

This is a completely random shot as I was just reading polars source code for fun. I can imagine the compiler optimises this anyway but I'd rule out 75 % of cases in the first check rather than 0.25 %.
@janpipek janpipek changed the title Update calendar.rs Reorder conditions in calendar.rs Nov 2, 2024
@janpipek janpipek changed the title Reorder conditions in calendar.rs fix(rust): Reorder conditions in is_leap_year Nov 2, 2024
@github-actions github-actions bot added fix Bug fix rust Related to Rust Polars and removed title needs formatting labels Nov 2, 2024
Copy link

codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.84%. Comparing base (1eb2fcc) to head (2fc93a8).
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #19602   +/-   ##
=======================================
  Coverage   79.84%   79.84%           
=======================================
  Files        1536     1536           
  Lines      211405   211405           
  Branches     2445     2445           
=======================================
+ Hits       168790   168797    +7     
+ Misses      42060    42053    -7     
  Partials      555      555           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ritchie46
Copy link
Member

@MarcoGorelli can you take a look?

@ritchie46 ritchie46 changed the title fix(rust): Reorder conditions in is_leap_year perf: Reorder conditions in is_leap_year Nov 4, 2024
@github-actions github-actions bot added performance Performance issues or improvements python Related to Python Polars labels Nov 4, 2024
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Nov 4, 2024

Makes sense to me that this will be faster; to codify the degree of speedup (and to confirm that the compiler isn't psychic) I checked it with a dist-release build - testing df.select(pl.col("dtm").dt.is_leap_year()) on ~9 million datetimes gets about 8% faster with the proposed change, so gets my vote 👌

Copy link
Collaborator

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

sure, thanks @janpipek , and @alexander-beedie for timing it!

@MarcoGorelli MarcoGorelli merged commit 8211845 into pola-rs:main Nov 5, 2024
22 of 24 checks passed
@janpipek
Copy link
Contributor Author

janpipek commented Nov 5, 2024

🙏 Thanks for approval and testing. I am glad that it really helped (8% is very nice!). As said, it was more of a random observation.

@janpipek janpipek deleted the patch-1 branch November 5, 2024 12:52
@alexander-beedie
Copy link
Collaborator

🙏 Thanks for approval and testing. I am glad that it really helped (8% is very nice!). As said, it was more of a random observation.

Thanks - feel free to make more ;)

tylerriccio33 pushed a commit to tylerriccio33/polars that referenced this pull request Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix performance Performance issues or improvements python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants