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
Improve find_ch_adjacency and read_ch_adjacency #12293
base: main
Are you sure you want to change the base?
Conversation
# TODO: This test is strange, raw and bti148 adjacency matrix do not have the same | ||
# channel names. Previously, it was loading the bti148 adjacency matrix without the | ||
# 'mag' channel selection confirming the existence of the channel names in the raw | ||
# object, and asserting: | ||
# assert 'A1' in ch_names # returned by find_ch_adjacency | ||
# while in practice, 'A1' is not in raw.ch_names | ||
bti_fname = testing_path / "BTi" / "erm_HFH" / "c,rfDC" | ||
bti_config_name = testing_path / "BTi" / "erm_HFH" / "config" | ||
raw = read_raw_bti(bti_fname, bti_config_name, None) | ||
_, ch_names = find_ch_adjacency(raw.info, "mag") | ||
assert "A1" in ch_names | ||
with pytest.raises(ValueError, match="could not be interpreted as channel names"): | ||
find_ch_adjacency(raw.info, "mag") | ||
|
||
# TODO: same as above, with 'assert "MLC11" in ch_names'. | ||
ctf_fname = testing_path / "CTF" / "testdata_ctf_short.ds" | ||
raw = read_raw_ctf(ctf_fname) | ||
_, ch_names = find_ch_adjacency(raw.info, "mag") | ||
assert "MLC11" in ch_names | ||
with pytest.raises(ValueError, match="could not be interpreted as channel names"): | ||
_, ch_names = find_ch_adjacency(raw.info, "mag") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Channel names in raw
and adjacency matrix do not match. What should be the correct and tested behavior here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try it with read_raw_bti(..., rename_channels=False)
? If so I think we might need to detect somehow the BTi-ness of the data (probably by MEG coil type) and internally be willing to tolerate the renaming to MEGXXX
-style naming when rename_channels=True
.
Fixes #12292
This PR:
ch_type
infind_ch_adjacency
in favor ofpicks
info
read_ch_adjacency
: thepicks
argument can not be a list of indices and can not interact with an non-existinginfo["bads"]
list.IMO, the test
test_find_ch_adjacency
has something wrong with thebti
andctf
segments. It loads adjacency matrix with different channel names than theraw recording
. Changes to pass the test highlighted below, any input would be welcome as I am unfamiliar with those 2 MEG systems.