search

mne-tools / mne-nirs

Process Near-Infrared Spectroscopy Data in MNE
https://mne.tools/mne-nirs/
BSD 3-Clause "New" or "Revised" License
84 stars 35 forks source link

Automatically sanitse annotation names that arent compatible with nilearn #403

Closed rob-luke closed 1 year ago

rob-luke commented 3 years ago

Whats the problem?

Nilearn recently changed the requirements for design matrix column names in https://github.com/nilearn/nilearn/pull/3025 which disallowed strings containing only numbers and strings with / as column names.

Getting rid of the numbers is no problem, I prefer meaningful annotation names anyway.

However, I think its common in MNE to use names like tapping/left or response/right. See https://mne.tools/dev/auto_tutorials/epochs/40_autogenerate_metadata.html?highlight=hed#keeping-only-the-first-events-of-a-group

But nilearn now requires that the column names pass the following test:

'response/right'.isidentifier()
# False

'response_right'.isidentifier()
# True

See lines: https://github.com/nilearn/nilearn/blob/ab9c68e502c4b92695cd658f5839a8207fd5e07e/nilearn/glm/first_level/hemodynamic_models.py#L428-L436

Whats the fix?

This PR checks that our design matrix column names are compatible with nilearn, and if they are not then prepends a t_ to force them to be valid.

The problem

This PR also changes / to _ in each design matrix column name, but this is not then harmonious with the rest of MNE. Further, it means that in some places of my script I use tapping/left and other places I will need to use tapping_left. This solution doesn't seem great, what is a better alternative?

rob-luke commented 3 years ago

@agramfort and @larsoner I am a bit stumped about what to do here. Nilearn has removed the ability to use / in design matrix columns. These columns represent experimental conditions and so I have previously just used the annotation name. However, annotation names commonly include / in them. Any fresh ideas of how I can tackle this problem?

rob-luke commented 3 years ago

I will pin nilearn to previous version rather than main until I know how to fix this.

larsoner commented 3 years ago

To me this seems like a bug / regression in nilearn. It's entirely possible they haven't considered spaces or / or number-first names to be widespread use cases, but clearly they are at least for us. I would open an issue and see if it's possible for them to fix -- to me it seems like they should sanitize the strings when doing whatever processing they need to do, and then restore them afterward...

If they don't like this solution, it's what I'd probably implement at the MNE-NIRS end.

rob-luke commented 3 years ago

We now know that we can not use the / symbol in the design matrix. So I will try and write some sort of sanitisation approach as suggested by larsoner and will use ___ to equal /.