-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: support weekend argument in business_day_count #15544
Conversation
7a42c53
to
7fff1e8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15544 +/- ##
==========================================
- Coverage 81.17% 81.16% -0.02%
==========================================
Files 1367 1367
Lines 175313 175321 +8
Branches 2530 2530
==========================================
- Hits 142314 142297 -17
- Misses 32524 32550 +26
+ Partials 475 474 -1 ☔ View full report in Codecov by Sentry. |
from polars.type_aliases import IntoExprColumn | ||
from polars.type_aliases import DayOfWeek, IntoExprColumn | ||
|
||
DAY_NAMES = ( |
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.
Can we put this behind a cached function? Small things to keep import times down. ^^
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.
thanks - I've ended up making it much simpler: users just pass an iterable of booleans, and that gets passed to the Rust side as-is
slightly less ergonomic perhaps, but I think realistically people would just define a constant with their week mask and pass that in, and the code's much simpler like this, sorry for having overcomplicated it
CodSpeed Performance ReportMerging #15544 will not alter performanceComparing Summary
|
part II of upstreaming business days to Polars
The "grand plan" is:
holidays
argument to this function (this one may stir up a little more discussion)add_business_day
, which would also takeweekend
andholidays
argumentsis_business_day
, which would also takeweekend
andholidays
arguments (this one should be trivial)