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.

Sign up for GitHub

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

Jump to bottom

[BUG] fix ForecastX when forecaster_X_exogeneous="complement" #6433

Merged
merged 9 commits into from
May 23, 2024
Merged

[BUG] fix ForecastX when forecaster_X_exogeneous="complement" #6433

merged 9 commits into from
May 23, 2024

Conversation

fnhirwa
Copy link
Contributor

@fnhirwa fnhirwa commented May 16, 2024 •
edited

Reference Issues/PRs

fixes #6405

What does this implement/fix? Explain your changes.

The issue was the way we were handling the complementation of the columns.

PR checklist

For all contributions
  • 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.

@fnhirwa fnhirwa marked this pull request as ready for review May 16, 2024 09:06
@fkiraly fkiraly changed the title [BUG] -bugfix ForecastX when forecaster_X_exogeneous="complement" [BUG] fix ForecastX when forecaster_X_exogeneous="complement" May 16, 2024
@fkiraly fkiraly added module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting bugfix Fixes a known bug or removes unintended behavior labels May 16, 2024
fkiraly
fkiraly requested changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

I think this fixes the bug.

Best practice is, every bugfix should contain a test or otherwise increase test coverage so the fixed bug would fail that test. That's sometimes called "certifying for the fix".

In most cases, adding (a simplified or quickened version of) the reproducible code in the bug report does the trick. In cases like estimators, you can work with adding parameter combinations to get_test_params. In rare cases, the test system needs to be extended, e.g., by addition of suite tests, if it turns out that an important use case was entirely overlooked - a recent examples is #6428

fkiraly
fkiraly requested changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Failure is due to sth flipping column order - I would guess it has to do with the use of set.

yarnabrina
yarnabrina requested changes May 17, 2024
View reviewed changes
sktime/forecasting/compose/_pipeline.py Outdated Show resolved Hide resolved
sktime/forecasting/compose/_pipeline.py Outdated Show resolved Hide resolved
sktime/forecasting/compose/_pipeline.py Outdated Show resolved Hide resolved
sktime/forecasting/compose/_pipeline.py Outdated Show resolved Hide resolved
fkiraly
fkiraly requested changes May 17, 2024
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Agree, there are many redundant checks. This should be resolved.

Also order can be preserved by list comprehension, agreed. That should solve the failure.

@fnhirwa
Copy link
Contributor Author

fnhirwa commented May 20, 2024

I think this fixes the bug.

Best practice is, every bugfix should contain a test or otherwise increase test coverage so the fixed bug would fail that test. That's sometimes called "certifying for the fix".

In most cases, adding (a simplified or quickened version of) the reproducible code in the bug report does the trick. In cases like estimators, you can work with adding parameter combinations to get_test_params. In rare cases, the test system needs to be extended, e.g., by addition of suite tests, if it turns out that an important use case was entirely overlooked - a recent examples is #6428

I agree with this but from my understanding this parameter was tested here

sktime/sktime/forecasting/compose/_pipeline.py

Line 1814 in ed0ee5b

"forecaster_X_exogeneous": "complement",

@yarnabrina
Copy link
Collaborator

The existing test parameters do not test this argument in presence of explicit mention of columns, either as list of strings or as index. I suspect that's how we didn't find it earlier, so adding the test is necessary.

Also, curious what is your point of view about L1578 above? I made a comment/question above.

@fnhirwa
Copy link
Contributor Author

fnhirwa commented May 20, 2024 •
edited

The existing test parameters do not test this argument in presence of explicit mention of columns, either as list of strings or as index. I suspect that's how we didn't find it earlier, so adding the test is necessary.

Also, curious what is your point of view about L1578 above? I made a comment/question above.

This was an overlook I am going to add a test case where we explicitly mention columns
Also, added my answer on the comment as well, it just slipped under my radar 🙂

@yarnabrina
Copy link
Collaborator

@fnhirwa Can I please request to keep the existing test as it is, and add a new test for forecaster_X_exogeneous? In future, it may help identifying what combination is failing exactly.

@fnhirwa
Copy link
Contributor Author

fnhirwa commented May 21, 2024

@fnhirwa Can I please request to keep the existing test as it is, and add a new test for forecaster_X_exogeneous? In future, it may help identifying what combination is failing exactly.

sure let me add the separate test for this

@fkiraly
Copy link
Collaborator

fkiraly commented May 22, 2024

why is the readthedocs build failing, any idea? Could you push again and restart?

@fnhirwa
Copy link
Contributor Author

fnhirwa commented May 22, 2024

why is the readthedocs build failing, any idea? Could you push again and restart?

It is probably a timeout error.
image
Pushed a commit to trigger the workflows again

@fkiraly
Copy link
Collaborator

fkiraly commented May 22, 2024

seems to have been a one-off...

yarnabrina
yarnabrina approved these changes May 22, 2024
View reviewed changes
Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

Approved in principle. Will merge when CI shows completed with full success.

@yarnabrina yarnabrina merged commit 0345ac0 into sktime:main May 23, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fkiraly fkiraly fkiraly approved these changes

@yarnabrina yarnabrina yarnabrina approved these changes

@achieveordie achieveordie Awaiting requested review from achieveordie achieveordie is a code owner

@benHeid benHeid Awaiting requested review from benHeid benHeid is a code owner

Assignees
No one assigned
Labels
bugfix Fixes a known bug or removes unintended behavior module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] ForecastX is failing with forecaster_X_exogeneous="complement"
3 participants
@fnhirwa @yarnabrina @fkiraly