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

Implement IsWorkingHours and IsLunchTime #2130

Merged
merged 69 commits into from
Jul 7, 2022
Merged

Implement IsWorkingHours and IsLunchTime #2130

merged 69 commits into from
Jul 7, 2022

Conversation

sbadithe
Copy link
Contributor

@sbadithe sbadithe commented Jun 22, 2022

Adds the following datetime transform primitives:
-- IsWorkingHour : Checks if hour is between configurable start and end times
-- IsLunchTime : Checks if hour is equal to configurable lunch time hour

Fixes #2076
Fixes #2078

@sbadithe
Copy link
Contributor Author

Should I include holiday checking for IsLunchTime?

@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #2130 (07012b5) into main (8a0d53f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 07012b5 differs from pull request most recent head 70a4384. Consider uploading reports for the commit 70a4384 to get more accurate results

@@           Coverage Diff           @@
##             main    #2130   +/-   ##
=======================================
  Coverage   99.23%   99.24%           
=======================================
  Files         143      143           
  Lines       17196    17245   +49     
=======================================
+ Hits        17065    17114   +49     
  Misses        131      131           
Impacted Files Coverage Δ
...imitives/standard/datetime_transform_primitives.py 100.00% <100.00%> (ø)
.../tests/primitive_tests/test_transform_primitive.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9de1ae...70a4384. Read the comment docs.

@sbadithe sbadithe marked this pull request as ready for review June 22, 2022 14:53
@sbadithe sbadithe marked this pull request as draft June 22, 2022 15:00
@gsheni
Copy link
Contributor

gsheni commented Jun 27, 2022

@sbadithe I would make include_holidays an optional argument, and default to False. You have lunch on the weekends :-)

@sbadithe
Copy link
Contributor Author

sbadithe commented Jun 27, 2022

@gsheni Would it make sense to include a weekday_only option too?

@sbadithe sbadithe closed this Jun 27, 2022
@sbadithe sbadithe reopened this Jun 27, 2022
@gsheni
Copy link
Contributor

gsheni commented Jun 28, 2022

@sbadithe yes

@sbadithe sbadithe marked this pull request as ready for review June 28, 2022 18:39
Examples:
>>> from datetime import datetime
>>> dates = [datetime(2022, 6, 21, 16, 3, 3),
... datetime(2019,1,3,4,4,4),
Copy link
Contributor

Choose a reason for hiding this comment

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

fix formatting here to include white space

ilt = IsLunchTime(include_weekends=False, include_holidays=True)
dates = pd.Series(
[
datetime(2022, 6, 26, 12, 12, 12),
Copy link
Contributor

@dvreed77 dvreed77 Jun 28, 2022

Choose a reason for hiding this comment

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

can you include a comment that is something like Wed, Jun 26 2022 - (Holiday Name) next to each of these examples everywhere

iwh = IsWorkingHours(15, 18)
dates = pd.Series(
[
datetime(2022, 6, 21, 16, 3, 3), # Weekday date
Copy link
Contributor

Choose a reason for hiding this comment

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

Include case outside working hours but on a weekday

@sbadithe sbadithe marked this pull request as draft June 28, 2022 23:08
@sbadithe
Copy link
Contributor Author

sbadithe commented Jul 6, 2022

Changes:

IsLunchTime : checks if dt.hour is equal to a configurable lunch time
IsWorkingHours : checks if dt.hour is between configurable start and configurable end hour

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Just a couple more minor things, but looking pretty good to me.

@gsheni gsheni requested review from dvreed77 and thehomebrewnerd and removed request for dvreed77 July 7, 2022 15:43
datetime(2022, 6, 21, 16, 3, 3),
datetime(2019, 1, 3, 4, 4, 4),
datetime(2022, 1, 1, 12, 1, 2),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

One last item - we should add a nan value here like we do in IsLunchHour just to confirm this primitive doesn't have issues with missing values.

thehomebrewnerd
thehomebrewnerd previously approved these changes Jul 7, 2022
Copy link
Contributor

@ozzieD ozzieD left a comment

Choose a reason for hiding this comment

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

besides Nate's last comment, LGTM.

@sbadithe sbadithe merged commit 6d526d0 into main Jul 7, 2022
@sbadithe sbadithe deleted the workday-primitives branch July 7, 2022 17:48
This was referenced Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add IsWorkingHours primitive Add IsLunchTime primitive
5 participants