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 flaky tests #197

Merged
merged 9 commits into from
Feb 6, 2025
Merged

Fix flaky tests #197

merged 9 commits into from
Feb 6, 2025

Conversation

EthanMarx
Copy link
Collaborator

@EthanMarx EthanMarx commented Feb 6, 2025

  • Fixes power law flaky test by removing initial guess
  • Fixes spectral density flaky tests by allowing at most 1 sample to mismatch by more than tolerance. This happens occasionally, at it appears to be for low values. Eventually, the underlying cause of this should be addressed.

@EthanMarx
Copy link
Collaborator Author

@wbenoit26 The power law test was fixed by removing the initial guess. Spectral density test fixed by allowing at most 1 sample to differ by more than tolerance. This is a bandaid solution, but considering its only ever 1 sample and not that often, I think its acceptable.

Ran the power law test 1000 and the spectral density 100 times with no errors

@EthanMarx EthanMarx requested a review from wbenoit26 February 6, 2025 17:01
@@ -17,10 +17,18 @@ def compare_against_numpy():

def compare(value, expected):
sigma = 0.01
prob = 0.9999
prob = 0.99999
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking into this the other day - changing the probability has only a small effect on the tolerance, and wouldn't have prevented most of the failed unit tests (at least, for the ones that I spot checked). Changing the sigma value would have a larger impact; however, it also seemed like the relative error had a much smaller variance than the sigma we use here (around 0.001 for the fast spectral density with y), which makes me wonder if there's something genuinely wrong with the function for some inputs. Or if there's something wrong with the logic of the test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that wasnt the thing that ended up fixing it. At least for test_spectral_density, I only ever saw 1 sample deviate beyond the tolerance, so I now just allow at most 1 sample to deviate. That lead to 100 runs through it succeeding.

I need to test the others test_fast_spectral_density and test_fast_spectral_density_with_y.
If those have other fundamental issues, I'll look into a more robust solution

Copy link
Contributor

Choose a reason for hiding this comment

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

test_fast_spectral_density_with_y seems to be where it fails most often. Can you try running that one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup running that now 100 times, close to completing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That one ran but iirfilter tests are flaky.....

Copy link
Contributor

Choose a reason for hiding this comment

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

True, and those tests do only one sample, so that doesn't bode well. There's some edge effects with this filter compared with SciPy, right? Maybe more of the edge needs to be sliced off.

Copy link

github-actions bot commented Feb 6, 2025

Coverage report

This PR does not seem to contain any modification to coverable code.

@EthanMarx
Copy link
Collaborator Author

@wbenoit26 The iirfilter is still flaky but don't want to hold these improvements up. I'll assign that to ravi

Copy link
Contributor

@wbenoit26 wbenoit26 left a comment

Choose a reason for hiding this comment

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

Sweet, this looks good to me

@EthanMarx EthanMarx merged commit 8755501 into ML4GW:main Feb 6, 2025
6 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.

2 participants