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

[BUG] skip sporadic test errors in ExponentialSmoothing #5516

Merged
merged 1 commit into from Jan 20, 2024

Conversation

achieveordie
Copy link
Collaborator

@achieveordie achieveordie commented Nov 2, 2023

Skips failures in #5026 by removing test case triggering it - does not solve the bug.

Due to the bug, ExponentialSmoothing's .predict() returns NaN sporadically, causing subsequent operations to fail since they expect finite values.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 2, 2023

one question is also, does this happen locally as well as remote, or only remote?

Locally, you can run a loop of 100 check_estimator-s with the offending test and see how often it happens.

@achieveordie
Copy link
Collaborator Author

I tried what you're suggesting with only 10 iterations but found no errors. I'll try increasing it.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 2, 2023

you can get more iterations in if you select the specific test

@achieveordie
Copy link
Collaborator Author

achieveordie commented Nov 2, 2023

Here are my findings:

  • The failing estimator is ExponentialSmoothing and the failing method is TestAllForecasters::test_score() (from the runners in the PR).
  • Every single call yields 12 tests in total(different param combinations etc), here's an example of one of those:
{'test_score[ExponentialSmoothing-0-y:1cols-fh=1]': 'PASSED', 'test_score[ExponentialSmoothing-0-y:1cols-fh=[2 5]]': 'PASSED', 'test_score[ExponentialSmoothing-0-y:2cols-fh=1]': 'PASSED', 'test_score[ExponentialSmoothing-0-y:2cols-fh=[2 5]]': 'PASSED', 'test_score[ExponentialSmoothing-1-y:1cols-fh=1]': 'PASSED', 'test_score[ExponentialSmoothing-1-y:1cols-fh=[2 5]]': 'PASSED', 'test_score[ExponentialSmoothing-1-y:2cols-fh=1]': 'PASSED', 'test_score[ExponentialSmoothing-1-y:2cols-fh=[2 5]]': 'PASSED', 'test_score[ExponentialSmoothing-2-y:1cols-fh=1]': 'PASSED', 'test_score[ExponentialSmoothing-2-y:1cols-fh=[2 5]]': 'PASSED', 'test_score[ExponentialSmoothing-2-y:2cols-fh=1]': 'PASSED', 'test_score[ExponentialSmoothing-2-y:2cols-fh=[2 5]]': 'PASSED'}
  • I ran the test for 1000 iterations getting 12000 total test runs, out of which 46 failed. Curiously, these failures were coming from only two combinations:
{'test_score[ExponentialSmoothing-2-y:1cols-fh=[2 5]]': 14, 'test_score[ExponentialSmoothing-2-y:2cols-fh=[2 5]]': 32}

(Note: ExponentialSmoothing-2 is the one that contains use_boxcox=True if the order of test params is considered the same order)

If my hypothesis is correct, this number should go down to zero after use_boxcox=False in test params. I'll soon update this comment after running the diagnosis script with new param set (will also add the script here for reference).

Edit: I can confirm that no tests fail after changing to use_boxcox=False. Here's my quick and dirty script that I've used, takes about 20 mins for 1000 iterations:

from sktime.utils.estimator_checks import check_estimator
from sktime.forecasting.exp_smoothing import ExponentialSmoothing
from tqdm import tqdm

ITERATIONS = 1000
result = []
for i in range(ITERATIONS):
    if i % 10 == 0:
        print("Iteration number: ", i)
    result.append(
        check_estimator(
            estimator=ExponentialSmoothing,
            tests_to_run="test_score",
            verbose=False
        )
    )

passed, failed = 0, 0
types_of_failures = {}

for i in result:
    for key, val in i.items():
        if val == "PASSED":
            passed += 1
        else:
            failed += 1
            types_of_failures[key] = types_of_failures.get(key, 0) + 1

print("=="*25)
print(passed, failed)
print("--"*25)
print(types_of_failures)

@achieveordie achieveordie changed the title [DIAG] Diagnose Sporadic Test Errors in ExponentialSmoothing [BUG] Diagnose and fix Sporadic Test Errors in ExponentialSmoothing Nov 2, 2023
@achieveordie achieveordie marked this pull request as ready for review November 2, 2023 17:12
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.

I am not sure if it's a bugfix. It uses a test case that passes, but does it resolve the bug that occasionally fails the other test case(s)?

@achieveordie
Copy link
Collaborator Author

I am not sure if it's a bugfix. It uses a test case that passes, but does it resolve the bug that occasionally fails the other test case(s)?

This fixes the CI errors that were prominent across different PRs by "fixing" the test params. The actual source of error is considered to be numerical instability arising from statsmodel, and according to the comments, is by nature of this model (and perhaps their implementation).

So any other solution, if any, lies beyond the scope of sktime.

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.

But that's not a "fix", in the sense that it just reduces test coverage away from the failure case? The failure case is still there, just no longer tested...

A "fix" would be sth, e.g., that does not reduce test coverage, but does sth to ensure we are interface compliant even in the sporadic failure case, such as filling zero, or restarting the fit, etc.

@achieveordie
Copy link
Collaborator Author

Hmm, I'm not sure if I agree with monkey-patching it. I agree with you and @yarnabrina (I think this is what he was implying too, I just misunderstood his statement) that this reduces the test coverage. Perhaps restarting the fit function should be a good alternative.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 4, 2023

That wouldnt be a monkey-patch, would it? Simply a fallback patch in case it would have failed.

@fkiraly fkiraly changed the title [BUG] Diagnose and fix Sporadic Test Errors in ExponentialSmoothing [BUG] skip sporadic test errors in ExponentialSmoothing Jan 20, 2024
@fkiraly fkiraly merged commit 7950ddb into sktime:main Jan 20, 2024
49 checks passed
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

3 participants