[BUG] Correcly set lagged variables to known when lag >= horizon

[hubkrieb]

Reference Issues/PRs

Fixes #1909

What does this implement/fix? Explain your changes.

This PR fixes how the lagged variables are assigned to known or unknown variables by setting the ones originating from known variables to known and the others to known only if lag >= horizon to avoid data leaks

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

I just checked it with the issue code snippet for now, but it might need a unit test to avoid changing its behaviour again

Any other comments?

PR checklist

• The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
• Added/modified tests
• Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with pre-commit install.
  To run hooks independent of commit, execute pre-commit run --all-files

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@b8dbacc). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1910   +/-   ##
=======================================
  Coverage        ?   86.36%           
=======================================
  Files           ?       96           
  Lines           ?     7801           
  Branches        ?        0           
=======================================
  Hits            ?     6737           
  Misses          ?     1064           
  Partials        ?        0           

Flag	Coverage Δ
cpu	86.36% <100.00%> (?)
pytest	86.36% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fkiraly]

Sounds good, well spotted!

If I may ask, how did you notice this?

[hubkrieb]

I had first implemented lagged features manually in my input data for one of my project and was worried about data leakage. When I noticed there was a lags parameter in TimeSeriesDataset I checked how they were handled and it didn't look right and coherent with the existing comment.

[fkiraly]

I see! Could we add that as a test then?

Given that we consider this a bug, we should add a test to prevent this from occurring later again.

[hubkrieb]

I just added a test to check the different cases mentioned in the tables of #1909

The only thing is I couldn't have a test that worked for a real variable that is not the target. I think it is because of the bug mentioned in #1587

[fkiraly]

Thanks!

[fkiraly]

Review of this would be appreciated, @fnhirwa, @phoeenniixx, @PranavBhatP

[PranavBhatP]

looks good!

[fnhirwa]

Looks good to me😊