fix: Add date_series as TimeGapSplit parameter and deprecate date_serie#784
Open
RedZapdos123 wants to merge 2 commits into
Open
fix: Add date_series as TimeGapSplit parameter and deprecate date_serie#784RedZapdos123 wants to merge 2 commits into
date_series as TimeGapSplit parameter and deprecate date_serie#784RedZapdos123 wants to merge 2 commits into
Conversation
Keep backward compatibility by accepting both date_series and date_serie while enforcing that only one is provided.\n\nAlso add regression tests for alias behavior and conflict validation, and update docs examples to use date_series.\n\nCloses koaning#761. Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
Author
|
@FBruzzesi I request a review for this PR (bug fix). |
FBruzzesi
reviewed
Apr 16, 2026
Collaborator
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Hey @RedZapdos123 👋🏼 thanks for your contribution. I added a couple of comments/suggestions to hopefully have more insightful messages for users
Reorder constructor/doc parameters so date_series is canonical and date_serie is legacy-last.\n\nUse structural pattern matching for (date_series, date_serie) handling, including clearer errors and DeprecationWarning on legacy-only usage.\n\nUpdate TimeGapSplit tests for warning/error paths and canonical keyword usage. Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
FBruzzesi
changed the title
date_series as TimeGapSplit parameter and deprecate date_serie
FBruzzesi
approved these changes
Apr 19, 2026
Collaborator
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @RedZapdos123 for the contribution and quick adjustment! It's a +1 from me 😉
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Issue #761 reported a typo in the
TimeGapSplitconstructor argument name, wheredate_seriewas used instead ofdate_series.This PR adds a backward compatible fix:
date_seriesdate_seriedate_seriesordate_serieis provideddate_serietodate_seriesTimeGapSplitdocs examples to usedate_seriesIt also adds regression tests that cover:
date_seriesworks correctlydate_seriesanddate_serieraises a clearValueErrorCloses #761.
Validation:
ruff check sklego/model_selection.py tests/test_model_selection/test_timegapsplit.pypytest -q tests/test_model_selection/test_timegapsplit.pypytest -qNote:
pytest -qreports one unrelated pre-existing failure intests/test_meta/test_grouped_predictor.py::test_custom_shrinkage. (See #785 )Type of change
Checklist
The validation screenshots of the tests run, locally on WSL: