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

Fix Issue #2577 - Zero Division Error in diagnostics.performance_metrics() causing failed assertion #2578

Merged
merged 4 commits into from May 18, 2024

Conversation

ThomasChia
Copy link
Contributor

@ThomasChia ThomasChia commented May 1, 2024

This PR resolves Issue #2577

This issue is caused by zero division leading to np.nan values, so this PR adds a fix by filling np.nan values with zero during the calculation of the SMAPE metric.

This PR also updates the test_performance_metrics() method in the TestPerformanceMetrics class in tests.test_diagnostics.py to cover all metrics handling zero values for y and yhat.

@@ -630,6 +630,7 @@ def smape(df, w):
Dataframe with columns horizon and smape.
"""
sape = np.abs(df['y'] - df['yhat']) / ((np.abs(df['y']) + np.abs(df['yhat'])) / 2)
sape = sape.fillna(0)

Choose a reason for hiding this comment

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

are there tests covering these methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a test looking at the mape metric underneath the wider test_performance_metrics() method, so I have extended that to test for zero values across all other metrics.

Choose a reason for hiding this comment

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

Great nice one

@tcuongd tcuongd self-requested a review May 18, 2024 12:08
@@ -234,13 +234,13 @@ def test_performance_metrics(self, ts_short, backend):
metrics=["coverage", "mse"],
)
assert set(df_horizon.columns) == {"coverage", "mse", "horizon"}
# Skip MAPE
df_cv.loc[0, "y"] = 0.0
# Handle zero y and yhat and skip MAPE
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can leave the MAPE skip test as-is and do the test for 0.0 y and 0.0 yhat afterwards.

I guess mdape doesn't have the same problem because it uses the rolling_median function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Yep, rolling_median doesn't get rid of the np.nan values, they're still included with the groupby.

@@ -236,11 +236,13 @@ def test_performance_metrics(self, ts_short, backend):
assert set(df_horizon.columns) == {"coverage", "mse", "horizon"}
# Skip MAPE
df_cv.loc[0, "y"] = 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll still want to keep these calls:

df_horizon = diagnostics.performance_metrics(
            df_cv,
            metrics=["coverage", "mape"],
        )
assert set(df_horizon.columns) == {"coverage", "horizon"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, missed that! Added back in now.

@tcuongd tcuongd merged commit 36421b7 into facebook:main May 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants