Skip to content

feat: enable to add weekends in DatetimeEncoder#2

Draft
MarieSacksick wants to merge 3 commits into
typofrom
add_weekends
Draft

feat: enable to add weekends in DatetimeEncoder#2
MarieSacksick wants to merge 3 commits into
typofrom
add_weekends

Conversation

@MarieSacksick

@MarieSacksick MarieSacksick commented May 8, 2026

Copy link
Copy Markdown
Owner

Description

Addresses skrub-data#710

I went for an attribute add_is_weekend which is a list[int].
Another option is possible. It would be to accept a boolean instead, and use the config to change what is considered a weekend and a weekday. I like it less for 2 reasons:

  • more difficult to change
  • and if the user has to deal with several datasets within the same session with different weekends, it's more painful to change config than a parameter (all the more in the same pipeline).

My option may seem weird and unintuitive to people who don't know that not the whole world have the same weekends (which is quite normal since almost everyone have the same weekends, cf wikipedia). I don't mind too much switching to a boolean.

None of these options are ok if, in the same dataset, there are several countries with different weekends, but, well, shouldn't be most cases, and this is intended to be "just" a helper, not to cover every possible option, isn't it?

Checklist

  • I have read the contributing guidelines
  • I have added tests that verify the bug fix
  • I have added an entry to CHANGES.rst describing the fix
  • My code follows the code style of this project
  • I have checked my code and corrected any misspellings

How Has This Been Tested?

AI Disclosure

  • This PR contains AI-generated code
    • I have tested the code generated in my PR
    • I have read and understood every line that has been generated by the AI agent
    • I can explain what the AI-generated code does

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.

1 participant