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

Consistently use the test sets as reference for gap_before and gap_after #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WenjieZ
Copy link
Owner

@WenjieZ WenjieZ commented Feb 11, 2022

There are two ways of defining a derived cross-validator. One is to redefine _iter_test_indices or _iter_test_masks (test viewpoint), and the other is to redefine _iter_train_masks or _iter_train_indices (train viewpoint).

Currently, these two methods assign different semantic meanings to the parameters gap_before and gap_after. The test viewpoint uses the test sets as the reference:

train    gap_before    test    gap_after    train

The train viewpoint uses the training sets as the reference:

test    gap_before    train    gap_after    test

This diverged behavior is not intended inappropriate. The package should insist on the test viewpoint, and hence this PR. It will be enforced in v0.2.

I don't think this issue has touched any users, for the derived classes in this package use _iter_test_indices exclusively (test viewpoint). No users have reported this issue either. If you suspect that you have been affected by it, please reply to this PR.

@WenjieZ WenjieZ added this to the v0.2 milestone Feb 11, 2022
@WenjieZ WenjieZ changed the title Switch gap_before and gap_after for _iter_complement_mask Consistently use the test sets as reference for gap_before and gap_after Feb 11, 2022
@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #45 (fb26eda) into master (2abbc3d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #45   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files           3        3           
  Lines         643      643           
=======================================
  Hits          627      627           
  Misses         16       16           
Impacted Files Coverage Δ
tscv/_split.py 93.77% <100.00%> (ø)
tscv/tests/test_split.py 99.74% <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 2abbc3d...fb26eda. Read the comment docs.

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.

None yet

1 participant