Repository: mne-tools/mne-bids
Pull request #1310 (2024-09-25 13:00:25)

Fix participant tsv values when `Info` contains weight and height information as numpy arrays
<!--
Thanks for contributing this pull request (PR).
If this is your first time, make sure to read
[CONTRIBUTING.md](https://github.com/mne-tools/mne-bids/blob/main/CONTRIBUTING.md)
-->

PR Description
--------------

We have raw fiff files at our institution that produce weight and height information as numpy arrays when read with the standard mne functions. These values are not unpacked so they end up as arrays in the participant.tsv. This also leads to wrong behavior when a second run for the same participant and session is written as the detection for already existing rows in participant.tsv then fails. Check the added test for an example and simulation.

This PR adds some sanitation to the handling of participant data, namely making sure that no nested lists/arrays are in there. If it does find a nested list/array and the inner array only has one item, that item gets pulled up. Otherwise, it throws an exception.

Merge checklist
---------------

Maintainer, please confirm the following before merging.
If applicable:

- [ ] All comments are resolved
- [ ] This is not your own PR
- [ ] All CIs are happy
- [ ] PR title starts with [MRG]
- [ ] [whats_new.rst](https://github.com/mne-tools/mne-bids/blob/main/doc/whats_new.rst) is updated
- [ ] New contributors have been added to [CITATION.cff](https://github.com/mne-tools/mne-bids/blob/main/CITATION.cff)
- [ ] PR description includes phrase "closes <#issue-number>"


hoechenberger: Hello, thanks for the contribution! I'm wondering if this shouldn't be addressed upstream in MNE-Python itself. Is this documented behavior? Seems like a bug to me tbh!
thht: Well, in my opinion, yes but who knows what kind of surprises are waiting considering the high number of formats that MNE-Python supports and the often non-standard implementations of vendors and libraries out there.

I'm happy to report and hunt this down in MNE-Python. But I think that if MNE-BIDS expects data to be of a certain format, it is a good idea to a) check if the data fulfills these requirements and b) fixes it if it is an easy fix. And this is basically all this PR does.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1310?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) Report
Attention: Patch coverage is `90.90909%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 97.38%. Comparing base [(`9575eab`)](https://app.codecov.io/gh/mne-tools/mne-bids/commit/9575eabd893002819a54d3128d923fcc69db1dca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) to head [(`6ae6384`)](https://app.codecov.io/gh/mne-tools/mne-bids/commit/6ae63848da8eecd7ab4f3eb0c5e74eb9a265e62c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools).

| [Files with missing lines](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1310?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) | Patch % | Lines |
|---|---|---|
| [mne\_bids/write.py](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1310?src=pr&el=tree&filepath=mne_bids%2Fwrite.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools#diff-bW5lX2JpZHMvd3JpdGUucHk=) | 83.33% | [2 Missing :warning: ](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1310?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1310      +/-   ##
==========================================
- Coverage   97.46%   97.38%   -0.08%     
==========================================
  Files          40       40              
  Lines        8781     8803      +22     
==========================================
+ Hits         8558     8573      +15     
- Misses        223      230       +7     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1310?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools).

hoechenberger: I generally agree, but I believe MNE-Python should do the validation and not allow for invalid stuff to appear in its Info structure. I'm surprised this can be even written to disk that way … So in any case, I'd like to escalate this to upstream just to be sure

cc @larsoner 
larsoner: Yes MNE-Python should make sure everything in `Info` conforms to the FIF standard at the very least with its own internal `read_raw_*` functions. So to me this does sound like a MNE-Python bug
thht: Yes, this should be validated in MNE-Python and yes, this is probably an MNE-Python bug. However, it is always the responsibility of the function that takes arguments to validate its arguments. Especially as MNE-BIDS does **not** explicitly rely on the reader functions of MNE-Python. The `write_raw_bids` takes a `mne.io.Raw` object as an input which intentionally allows users to manipulate its fields, including the entries of the `Info` dict.
larsoner: Agreed that inputs must be validated but I think for MNE-BIDS this should be essentially `assert isinstance(raw, mne.io.BaseRaw)` and if it is then it should reasonably assume that MNE-Python has validated that whatever `BaseRaw` subclass is being used, plus its attributes (including `Info`), are constructed and manipulated correctly before MNE-BIDS ever sees them. And it should be MNE-Python's job to (as much as possible) ensure that the classes are instantiated and manipulated properly. If you try to draw the line elsewhere -- like by having MNE-BIDS validate that the `Info` object in MNE-Python is correct -- you end up in a pretty deep rabbit hole I think.

Even if you don't buy this argument, there is at least a practical one in that MNE-Python already has a bunch of Info-validation stuff built into it already, so from that standpoint it makes more sense (more DRY) to continue implementing checks there rather than here.
sappelhoff: Thanks a lot @thht for this report!

I agree with eric to have the "core validation" in BaseRaw at mne, and then a basic check in mne-bids.
thht: The thing is, I can do something like this:

```python
raw = mne.io.read_raw('valid_data.fif');
raw.info['subject_data']['weight'] = np.array([np.random.rand((200, 20))])

write_raw_bids(raw, ....)
```

And the current implementation would just process it.

AFAIK, there is no proper and feasible way for MNE-Python to make sure that the `Info` object is always valid. I remember there to be some undocumented validation functions, but these are a) undocumented (if they are there in the first place) and b) not enforced.

And the point is not to validate the `Info` object in MNE-BIDS (although if there was a validation function, MNE-BIDS should use it). The point is to validate the data MNE-BIDS produces, uses to process the data and writes to disk .

In this particular case, the non-validation leads to a quite confusing error message that the participant is already in the participant file. Which is true but the specified behavior in that case is that if all information about the participant matches, there is no error. Adding this bit of validation produces either the documented behavior or a more meaningful error message.
larsoner: > AFAIK, there is no proper and feasible way for MNE-Python to make sure that the Info object is always valid. 

We have some processes in place in MNE-Python that at move us in this direction. They are not perfect but incremental progress is usually made when bugs like this show up actually! So at the very least I think we should try to extend these at the MNE-Python end and see how far we can get.

>  these are a) undocumented (if they are there in the first place) and b) not enforced.

The idea is that they shouldn't need to be documented because the enforcement should automagically happen. And enforcement does happen for some operations, e.g.:
```
>>> info["description"] = []
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/larsoner/python/mne-python/mne/_fiff/meas_info.py", line 1602, in __setitem__
    val = self._attributes[key](
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/larsoner/python/mne-python/mne/_fiff/meas_info.py", line 997, in _check_description
    _validate_type(description, (None, str), "info['description']")
  File "/home/larsoner/python/mne-python/mne/utils/check.py", line 642, in _validate_type
    raise TypeError(
TypeError: info['description'] must be an instance of None or str, got <class 'list'> instead.
```
or for something you shouldn't be able to set:
```
>>> info["sfreq"] = "a"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/larsoner/python/mne-python/mne/_fiff/meas_info.py", line 1600, in __setitem__
    raise RuntimeError(self._attributes[key])
RuntimeError: sfreq cannot be set directly. Please use method inst.resample() instead.
```
etc. So in this sense I think we "just" need to extend this logic a bit further to the `subject_info` field. There is at least some rudimentary validation already present:
```
>>> info["subject_info"] = []
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/larsoner/python/mne-python/mne/_fiff/meas_info.py", line 1602, in __setitem__
    val = self._attributes[key](
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/larsoner/python/mne-python/mne/_fiff/meas_info.py", line 1022, in _check_subject_info
    _validate_type(subject_info, (None, dict), "subject_info")
  File "/home/larsoner/python/mne-python/mne/utils/check.py", line 642, in _validate_type
    raise TypeError(
TypeError: subject_info must be an instance of None or dict, got <class 'list'> instead.
```
So there is probably a placeholder stub where we can already start working this validation in.
drammock: generally I agree with @larsoner that new validation code belongs in MNE-Python.  But if there are already files out there in the wild that have array-like values in the `subject_data` dict (as @thht demonstrated is possible [above](https://github.com/mne-tools/mne-bids/pull/1310#issuecomment-2374156529)) then I see no reason not to do some small things in MNE-BIDS to e.g. silently allow singleton arrays.
hoechenberger: We could easily validate (in MNE) at runtime with Pydantic. So saying that this is not possible is not quite right. 
thht: No, this is not my point. My point is that if a proper interface is expected, it must be enforced by the software accepting the data. It's the prerogative of the software what to do in case of a violation. It can be a proper error message, or doing a workaround with our without a warning.
At the moment, this leads to unspecified behavior within MNE-BIDS, i.e. an error that the subject is already present plus the suggestion to enable overwritting the file.

My entirely subjective opinion is that in this case, the interface violation could be handled within MNE-BIDS, because it's an easy and effective fix that might handle similar errors as well and increase the user experience with minimal risk of creating new unwanted behavior. Yet, I totally get the argument that MNE-BIDS should only accept valid inputs and not correct invalid ones. That is your choice, of course.

In any case, my argument is that if MNE-BIDS expects data in a certain format, it is of course the responsibility of the provider to ensure conformity. But it is still the responsibility of the data sink to make sure the data is valid. Because it's correct to require conformity but you can never trust that incoming data meets it.
hoechenberger: I suppose I'm against special-casing your specific use case. I've never seen this problem before and I wonder where one would stop? I'd argue that there's a bug in your code and the proper way to prevent this would be by improving MNE-Python; and to prevent writing invalid BIDS, MNE-BIDS must check the outputs it produces more carefully; but it's not MNE-BIDS's responsibility to ensure that an upstream package uses its own API correctly
hoechenberger: That said, I really appreciate your input; I just don't think it's the best approach for this specific problem. But in the end it's @sappelhoff who will have to decide 😃 
thht: I'm pretty sure it's not my coffee because all it did is that the file extract some information and send it to write_raw_bids. The code should never touch the info property. Might be some side effect, sure. Gonna check tomorrow to be sure. 

Anyhow, I can agree with you that MNE-BIDS should not correct the wrong input. I still think that the error message should be improved as what it reports at the moment is not what the issue is. 

If you find a consensus among yourselves, I'm more than happy to alter the code so that it checks what it wants to hand off to the participants tsv writing function and just produces a better error message.

And thanks! I appreciate your work on this and I also enjoy this constructive, respectful discussion on different opinions 😀
drammock: > My understanding is that you're essentially saying that there is no need to enforce a proper interface, with which I strongly disagree.

[...]

> I suppose I'm against special-casing your specific use case.

This comes across as a bit harsh to me.  If I look back at the very first sentence of the PR description, @thht says

> We have raw fiff files at our institution that produce weight and height information as numpy arrays when read with the standard mne functions.

...which doesn't sound like a case where the user wants to do something unusual / out-of-spec.

That said, I'm actually going to take back my earlier statement that adding more permissivity *to MNE-BIDS* might be alright in this case.  The right fix is to adjust the MNE-Python reader function(s) (and possibly to make the MNE-BIDS error message more informative/accurate, e.g., by checking for nested arrays in the relevant fields of `participants.tsv` when it's first read, and warning/erroring earlier? --- instead of waiting until a second run happens to crash).

sappelhoff: > If you find a consensus among yourselves

I think we have found that consensus: We want to improve the way `mne` validates the `info["subject_info"]` data structure, so that all downstream tools using `mne` (of which there are a lot) can directly benefit.

I would be happy to include additional checks in `mne_bids` in case they are needed in a BIDS context, or in case the checks (for whatever reason) have to be more general in `mne` than is useful for `mne_bids`.

Having said all of this, I personally do not have the capacity to work on the validation in `mne`, so I hope that you can be convinced to tackle it @thht.

If you can't and nobody else can for the forseeable future, I would be happy to merge the present fix as an interim solution. 

So to move on:

1. am I right that we found a consensus (see beginning of message) ?
2. would you be willing to contribute the fix to `mne` @thht ?
hoechenberger: I'm totally fine with merging this as-is if as long as we decide we should address this problem upstream sometime in the future. 👍
sappelhoff: I have restarted the CIs now that:

- https://github.com/mne-tools/mne-python/issues/12874

has been merged. 

Once this PR has been adjusted to the new state of things in mne, we can merge it!
thht: Hi! Sorry, I had some work that came up and was super urgent. I'm taking care of this now.
larsoner: > What exactly does this PR do, now that mne does not allow weight and height to be arrays anymore?

At least for MNE < 1.9 it will do some helpful validation

Patch:
diff --git a/doc/whats_new.rst b/doc/whats_new.rst
index 42a42a72..f495df82 100644
--- a/doc/whats_new.rst
+++ b/doc/whats_new.rst
@@ -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
 ^^^^^^^^^^^^^^
diff --git a/mne_bids/write.py b/mne_bids/write.py
index b7cf34d0..500d300f 100644
--- a/mne_bids/write.py
+++ b/mne_bids/write.py
@@ -511,6 +511,29 @@ def _participants_tsv(raw, subject_id, fname, overwrite=False):
         }
     )
 
+    # XXX: Remove once MNE-Python <1.9 is no longer supported
+    # Make sure that all entries to data are lists that
+    # contain scalars (i.e. not further lists). Fix if possible
+    for key in data.keys():
+        cur_value = data[key]
+
+        # 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. "
+                        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


Test patch:
diff --git a/mne_bids/tests/test_write.py b/mne_bids/tests/test_write.py
index 63485cda..e1f36974 100644
--- a/mne_bids/tests/test_write.py
+++ b/mne_bids/tests/test_write.py
@@ -4187,3 +4187,48 @@ 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
+
+
+# XXX: Remove once MNE-Python <1.9 is no longer supported
+@testing.requires_testing_data
+def test_write_bids_with_age_weight_info(tmp_path, monkeypatch):
+    """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)
+    # disable MNE-Python 1.9+ validation for the duration of this test
+    if "subject_info" in getattr(mne.Info, "_attributes", {}):
+        monkeypatch.setitem(
+            mne.Info._attributes,
+            "subject_info",
+            lambda val, **kwargs: val,
+        )
+    raw.info["subject_info"] = {
+        "weight": np.array([75.0]),
+        "height": np.array([180.0]),
+    }
+
+    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]),
+        "height": np.array([180.0]),
+    }
+
+    bids_path = _bids_path.copy().update(root=bids_root, datatype="meg", run=3)
+    assert_array_equal(raw.info["subject_info"]["weight"], [75.0, 10.2])
+    with pytest.raises(ValueError, match="more than one element"):
+        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)