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[tests]: increase overlap threshold in test_mode_solver_data_sort #2229

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

groberts-flex
Copy link
Contributor

This test has been failing sporadically. I was able to reproduce the issue locally when testing just this function instead of the whole test_monitor_data.py file. Changing the rng seed or state would then randomly cause the test to start working again. I traced this to the unsorting array that was getting generated (line 542 in the code change). When the random permutation was [3, 1, 2, 0] (i.e. - the first and last modes were swapped), they were not getting re-sorted properly. I was able to reproduce reliably by just hardcoding the permutation to this for all the frequencies.

It seems like the default overlap_thresh argument to the overlap_sort function is not high enough to properly sort these modes. Here, I've specified a higher overlap_thresh value in order for the test to pass assuming that the threshold being too low is mostly related to the test operating on generated data instead of actual mode solver runs. However, I'm wondering if this is an indication that the default should be made higher instead?

@yaugenst-flex
Copy link
Collaborator

Thanks @groberts-flex this is great, and it reveals another issue with our tests... all tests should be using this fixture. Instead, currently we are setting global rng seeds in multiple files, causing tests to not be deterministic due to parallel testing. And yeah the fake data should probably be more robust also...

Copy link

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Oh I see. Looks good as a patch for now.

@momchil-flex momchil-flex merged commit 5c7ec33 into pre/2.8 Feb 7, 2025
15 checks passed
@momchil-flex momchil-flex deleted the groberts-flex/fix_sorted_monitor_data_test branch February 7, 2025 13:07
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.

3 participants