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 participant tsv values when Info contains weight and height information as numpy arrays #1310

Merged
merged 13 commits into from
Oct 3, 2024
1 change: 1 addition & 0 deletions doc/whats_new.rst
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Detailed list of changes

- When anonymizing the date of a recording, MNE-BIDS will no longer error during `~mne_bids.write_raw_bids` if passing a `~mne.io.Raw` instance to ``empty_room``, by `Daniel McCloy`_ (:gh:`1270`)
- Dealing with alphanumeric ``sub`` entity labels is now fixed for :func:`~mne_bids.write_raw_bids`, by `Aaron Earle-Richardson`_ (:gh:`1291`)
- When processing subject_info data that MNE Python imports as numpy arrays with only one item, MNE-BIDS now unpacks these, resulting in a correct participants.tsv, by `Thomas Hartmann`_ (:gh:`1310`)

⚕️ Code health
^^^^^^^^^^^^^^
Expand Down
37 changes: 37 additions & 0 deletions mne_bids/tests/test_write.py
Original file line number Diff line number Diff line change
Expand Up @@ -4187,3 +4187,40 @@ def test_write_evt_metadata(_bids_validate, tmp_path):
for cur_col in event_metadata.columns:
assert cur_col in events_tsv
assert cur_col in events_json


sappelhoff marked this conversation as resolved.
Show resolved Hide resolved
@testing.requires_testing_data
def test_write_bids_with_age_weight_info(tmp_path):
"""Test writing participant.tsv when using np.arrays for weight and height."""
bids_root = tmp_path / "bids"
raw_fname = data_path / "MEG" / "sample" / "sample_audvis_trunc_raw.fif"
raw = _read_raw_fif(raw_fname)
raw.info["subject_info"] = {
"weight": np.array([75.0]),
"height": np.array([180.0]),
}
Comment on lines +4206 to +4209
Copy link
Member

Choose a reason for hiding this comment

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

I actually just thought of an issue... whenever mne-tools/mne-python#12874 is tackled (which I was about to try to see if it's trivial!) this will raise a ValueError. So I think you need to bypass the soon-to-be-added MNE protection:

Suggested change
raw.info["subject_info"] = {
"weight": np.array([75.0]),
"height": np.array([180.0]),
}
# bypass MNE-Python setter protection
dict.__setitem__(
raw.info,
"subject_info",
{
"weight": np.array([75.0]),
"height": np.array([180.0]),
},
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid this does not do the trick because while the initial setting works, the write_raw_bids function copies the raw object here. And this calls the validation again. I have not found any way around it, including trying to mock the __setitems__ method to be the original one from dict.

Copy link
Member

Choose a reason for hiding this comment

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

Pushed a commit using monkeypatch that should work

Copy link
Member

Choose a reason for hiding this comment

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

so that monkeypatch will essentially always have to be there now?

Copy link
Member

Choose a reason for hiding this comment

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

For the test to make sure that MNE-BIDS has its own validation, yes


bids_path = _bids_path.copy().update(root=bids_root, datatype="meg", run=1)
write_raw_bids(raw, bids_path=bids_path)
bids_path = _bids_path.copy().update(root=bids_root, datatype="meg", run=2)
write_raw_bids(raw, bids_path=bids_path)

# Test that we get a value error when we have more than one item
raw.info["subject_info"] = {
"weight": np.array([75.0, 10.2]),
larsoner marked this conversation as resolved.
Show resolved Hide resolved
"height": np.array([180.0]),
}

with pytest.raises(ValueError):
larsoner marked this conversation as resolved.
Show resolved Hide resolved
bids_path = _bids_path.copy().update(root=bids_root, datatype="meg", run=3)
write_raw_bids(raw, bids_path=bids_path)

# Test that scalar data is handled correctly

raw.info["subject_info"] = {
"weight": 75.0,
"height": np.array([180.0]),
}

bids_path = _bids_path.copy().update(root=bids_root, datatype="meg", run=3)
write_raw_bids(raw, bids_path=bids_path)
21 changes: 21 additions & 0 deletions mne_bids/write.py
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,27 @@ def _participants_tsv(raw, subject_id, fname, overwrite=False):
}
)

# Make sure that all entries to data are lists that
sappelhoff marked this conversation as resolved.
Show resolved Hide resolved
# contain scalars (i.e. not further lists). Fix if possible
for key in data.keys():
cur_value = data[key]
Comment on lines +517 to +518
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for key in data.keys():
cur_value = data[key]
for key, cur_value in data.items():

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that I do:

data[key] = new_value

within the loop. And afaik, one should never change what is iterated over. It might be save here, because care is taken that only the current entry is changed... But I think, it is safer this way....


# Check if all values are scalars
new_value = []
for cur_item in cur_value:
if isinstance(cur_item, list | tuple | np.ndarray):
if len(cur_item) == 1:
new_value.append(cur_item[0])
else:
raise ValueError(
f"Value for key {key} is a list with more "
f"than one element. This is not supported."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"than one element. This is not supported."
f"than one element. This is not supported. "
f"Got: {cur_value}"

)
else:
new_value.append(cur_item)

data[key] = new_value

if os.path.exists(fname):
orig_data = _from_tsv(fname)
# whether the new data exists identically in the previous data
Expand Down
Loading