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

Simplify CQT tests #1176

Closed
bmcfee opened this issue Jun 22, 2020 · 1 comment · Fixed by #1323
Closed

Simplify CQT tests #1176

bmcfee opened this issue Jun 22, 2020 · 1 comment · Fixed by #1323
Labels
good for beginners Are you new here? These issues are for you! testing Issues with our test design and continuous integration services
Milestone

Comments

@bmcfee
Copy link
Member

bmcfee commented Jun 22, 2020

At present, our test suite takes somewhere in the neighborhood of 15 minutes to run (on Travis). A substantial portion of that is due to the CQT tests, which brute-force through a very large parameter grid.

Rather than trying all combinations of parameters, we should refactor the tests to focus on the behavior of individual parameters (or small grids of parameters where interactions are inevitable).


For reference, here's the runtime on my dev machine for the full test suite:

1 failed, 12963 passed, 1 skipped, 466 xfailed, 10394 warnings in 849.78s (0:14:09)

and for just the cqt module:

2528 passed, 16 xfailed, 8593 warnings in 519.08s (0:08:39)
@bmcfee bmcfee added the testing Issues with our test design and continuous integration services label Jun 22, 2020
@bmcfee bmcfee added this to the 0.8.1 milestone Jun 22, 2020
@lostanlen lostanlen added already fixed Problems that we've already solved good for beginners Are you new here? These issues are for you! and removed already fixed Problems that we've already solved labels Jun 24, 2020
bmcfee added a commit that referenced this issue Jun 24, 2020
bmcfee added a commit that referenced this issue Jun 25, 2020
* fixed #1170 and #1176

* fixed docstring for chroma_cqt

* updated chroma notebook to reflect new defaults

* added safety check and test for chroma_cqt
@bmcfee bmcfee changed the title Simplfy CQT tests Simplify CQT tests May 8, 2021
@bmcfee
Copy link
Member Author

bmcfee commented May 8, 2021

Let's make some sense of this by first mapping out what the current test sweeps are. To keep it simple, I'm not counting all of our specific test cases, only the broadest tests with huge parameter grids.

`test_cqt` sweeps 9 parameters (8 non-trivial), 768 tests:
  • fmin (x2)
  • n_bins (x4)
  • bins_per_octave (x2)
  • tuning (x3)
  • filter_scale (x2)
  • norm (x2)
  • res_type (x2)
  • sparsity (x1)
  • hop_length (x2)
`test_vqt` sweeps 10 (4 non-trivial), 24 tests
  • fmin (x2)
  • n_bins (x2)
  • gamma (x3)
  • bins_per_octave (x2)
  • tuning (x1)
  • `filter_scale (x1)
  • norm (x1)
  • res_type (x1)
  • sparsity (x1)
  • hop_length (x1)
`test_hybrid_cqt` sweeps 10 (7 non-trivial), 672 tests
  • sr (x1)
  • hop_length (x1)
  • sparsity (x1)
  • fmin (x2)
  • n_bins (x7)
  • bins_per_octave (x2)
  • tuning (x3)
  • resolution (x2)
  • norm (x2)
  • res_type (x2)
`test_icqt` sweeps 6, 96 tests
  • over_sample (x2)
  • scale (x2)
  • hop_length (x2)
  • length (x2)
  • res_type (x3)
  • dtype (x2)
`test_griffinlim_cqt` sweeps 12 parameters (9 non-trivial), 768 tests
  • hop_length (x2)
  • window (x2)
  • use_length (x2)
  • over_sample (x2)
  • res_type (x1)
  • pad_mode (x1)
  • scale (x2)
  • momentum (x2)
  • random_state (x3)
  • fmin (x1)
  • dtype (x2)
  • init (x2)

In most of these cases, the parameters can be broken down into interacting sets that affect either A) the quality of the filters (res_type, filter_scale, sparsity, norm), B) the output frequency range (bins_per_octave, fmin, n_bins), C) frame rate (hop_length, sr). Although there are certainly some interactions here (eg between frequency range and frame rate), I think we can factor the tests to focus on specific aspects.

  • test_cqt()/test_vqt() currently only does two things in terms of validation: check that the output is complex, and check that the number of output bins is as expected. All the parameter sweeps do is ensure that various code paths are covered. (That is, it's an extremely weak test!)

  • test_hybrid_cqt does a bit more work, in that it's actually comparing the hybrid to full cqt implementations.

  • test_griffinlim_cqt is basically an integration test which ensures that output is correctly typed and finite.

  • test_icqt is arguably doing the most work, in that it checks types, lengths, and residual error on reconstruction. It also has the smallest sweep, so I'm inclined to leave it alone.

We should focus on reducing the number of tests for test_cqt, test_hybrid_cqt, and test_griffinlim_cqt. (VQT and ICQT are pretty minimal, all things considered.)

For test_cqt, we should split into a few different cases:

  • one that focuses on the filterbank configuration (n_bins, bins_per_octave, tuning, fmin) and checks that a pure tone input produces a peak in the appropriate location
  • one that focuses on frame rates (hop_length)
  • one-off tests for comparing res_type, norm, sparsity, and filter_scale
  • The above would take us from 768 to something like 48 + 2 + 2 + 2 + 2 + 2 = 58.

For hybrid_cqt, I think we can keep the structure mostly the same, and reduce the number of parameters we sweep over. (Eg, eliminate resolution, res_type, norm from the sweeps.) This could take us from 672 ==> 84 cases.

For griffinlim_cqt, I think we can isolate most of the parameters here:

  • one test with/without momentum
  • one test with different initializations
  • one test with different RNG seed formats
  • one test for the rest of the params. This would take us from 768 ==> 768//12 + 2 + 2 + 3 = 71

With the above modifications, we ought to be able to reduce the runtime for test_constantq.py by a factor of about 10 (10 minutes => 1 minute, on my dev machine) without compromising in terms of test coverage or quality control.

bmcfee added a commit that referenced this issue May 9, 2021
bmcfee added a commit that referenced this issue May 9, 2021
* added coverage.xml to gitignore

* accelerated some cqt tests

* fixes #1176; simplifies CQT test suite

* switched default cqt test to res_type=none instead of polyphase to trigger early downsampling

* streamlining cqt tests some more

* blacked test_constantq.py

* fixed early downsampling test coverage for cqt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good for beginners Are you new here? These issues are for you! testing Issues with our test design and continuous integration services
Development

Successfully merging a pull request may close this issue.

2 participants