Refactor of NoisyChannels tests and split into separate methods

[a-hurst]

PR Description

This PR takes the test_findnoisychannels function and splits it into separate unit tests for each type of bad channel, making it easier to maintain and figure out which specific part(s) are broken by a given change.

I also changed the test file from the current (very noisy) one to one with only two noisy channels to start with, saving time repeatedly running NoisyChannels and interpolating bads at the start to try and produce a clean recording for subsequent tests. This also has the advantage of not depending on NoisyChannels working properly in order to produce a dataset suitable for testing NoisyChannels. To address #83 I manually set the Z threshold to 3.29 (as suggested) and added a note so we don't forget.

Let me know what you think!

Merge Checklist

• the PR has been reviewed and all comments are resolved
• all CI checks pass
• (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
• (if applicable): bug fixes, new features, or API changes are documented in whats_new.rst

[codecov-commenter]

Codecov Report

Merging #84 (616c057) into master (bc46978) will not change coverage.
The diff coverage is n/a.

❗ Current head 616c057 differs from pull request most recent head b9ac92e. Consider uploading reports for the commit b9ac92e to get more accurate results

@@           Coverage Diff           @@
##           master      #84   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files           7        7           
  Lines         677      677           
=======================================
  Hits          666      666           
  Misses         11       11           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc46978...b9ac92e. Read the comment docs.

[a-hurst]

Huh, looks like this had the unintended effect of making other sections of the test suite slower? Looking back at the old tests it looks like the current test_findnoisychannels makes direct modifications to the raw fixture that's used across tests, such that the PrepPipeline and Reference tests were taking a modified raw as input:

pyprep/tests/test_find_noisy_channels.py

Lines 15 to 32 in bc46978

	raw.set_montage(montage)
	nd = NoisyChannels(raw, random_state=rng)
	nd.find_all_bads(ransac=True)
	bads = nd.get_bads()
	iterations = (
	10 # remove any noisy channels by interpolating the bads for 10 iterations
	)
	for iter in range(0, iterations):
	if len(bads) == 0:
	continue
	raw.info["bads"] = bads
	raw.interpolate_bads()
	nd = NoisyChannels(raw, random_state=rng)
	nd.find_all_bads(ransac=True)
	bads = nd.get_bads()
	# make sure no bad channels exist in the data
	raw.drop_channels(ch_names=bads)

Since the above code tries to clean bad channels from the recording, I'm guessing that meant it took fewer iterations to re-reference during later tests. Regardless, I doubt that was intentional so I guess this PR fixes that as well!

[sappelhoff]

I am surprised that BLACK does not complain here. It usually formats all quotation marks as " instead of ', right?

[a-hurst]

I honestly don't know, I haven't really used black formatting before! If you have a preference for double quotes over singles for this project I'll definitely stick to that.

[sappelhoff]

I mostly have a preference for using a consistent style - and preferably one that is enforced by a tool with a large community backing, such as black :-)

But if it doesn't enforce a format here, it's fine with me 👍

[a-hurst]

From the look of it, PyPREP doesn't have any sort of automatic linting for Black enabled other than the 88 line length, is that something that should be fixed?

And that make sense! My general habit to use single-quotes for data (e.g., variable names, argument strings, key names, etc.) and double-quotes for strings that are meant to be read as text (e.g. docstrings, print messages, exception messages, etc.), but I'm happy to conform to whatever conventions a project uses. From what I can tell the Black style guide prefers double-quotes to singles (like you thought), so up to you.

[sappelhoff]

argh, good point actually. This line is supposed to ensure in CI that all code is BLACK formatted:

pyprep/.github/workflows/python_tests.yml

Line 41 in bc46978

black .

But it should say black . --check instead of black . --> the former does a check, whereas the latter just applies the formatting, which is then obviously lost because it's not committed.

And for devs, the development environment should be setup automatically such that BLACK formatting is applied prior to each commit (pre-commit hook, see https://github.com/sappelhoff/pyprep#installation)

I'll make a PR for that.

[sappelhoff]

see: #85

[sappelhoff]

LGTM in general :) again -> a very helpful addition, thanks!

I left a few comments, but should be easy to address overall and then get it merged.

[sappelhoff]

Thanks! I'll fix the formatting in #85 and make sure that the CI will check this in the future.