MRG: Add sessions as official suffix

[JojoVh]

PR Description

Addition of sessions as a suffix when reading BIDS dataset.
Sessions are a suffix since BIDS v1.8
See:
https://bids-specification.readthedocs.io/en/stable/glossary.html#sessions-suffixes

I tested the code to successfully read in sessions.tsv and sessions.json files.

best wishes
Jonathan Vanhoecke
ICN Lab - Charité Universitätsmedizin Berlin

[welcome]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

[codecov]

Codecov Report

Merging #1137 (1933918) into main (9394395) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1137   +/-   ##
=======================================
  Coverage   95.28%   95.28%           
=======================================
  Files          40       40           
  Lines        8694     8695    +1     
=======================================
+ Hits         8284     8285    +1     
  Misses        410      410           

Impacted Files	Coverage Δ
mne_bids/config.py	97.67% <ø> (ø)
mne_bids/tests/test_read.py	98.47% <ø> (ø)
mne_bids/tests/test_path.py	99.84% <100.00%> (+<0.01%)	⬆️

[sappelhoff]

agreed, sessions.tsv and sessions.json are valid BIDS files.

[sappelhoff]

@JonathanVHoecke I restarted the CI and apart from the doc-build, everything passes now. No idea why, but circleci somehow blocks execution for your PR.

Could you please elaborate on what your changes enable, why you need that, and what other adjustments may be needed (if any)?

Thanks!

[hoechenberger]

We will also need to add a test case before merging 👍

[JojoVh]

Hi @sappelhoff and @hoechenberger

My change was very small by just adding "sessions" into mne_bids/config.py under the allowed suffixes. No other changes were needed.
Why I need that, is because we use the sessions.tsv file to store session-specific information, such as the UPDRS score and other clinical scores as measured on that day.
I did not see any other need for other changes.

For the test-case, I added a sessions.tsv and sessions.json file in your tiny_bids dataset.
Further, I recycled your test_handle_scans_reading to be the same for test_handle_sessions_reading

I hope this is an answer to your question
best wishes
Jonathan

[sappelhoff]

Thanks for the explanations :-) I don't really get the test case --> if sessions.tsv wasn't even considered a valid file for mne-bids until this PR, how can you add a test case that requires mne-bids to take changes in some sessions.tsv file into account? Or do I misunderstand something here?

Furthermore, could you please take care that the CI checks pass? (Except for the doc build, which --as said before-- somehow does not trigger for you)

[hoechenberger]

Regarding my proposal to add a test, I was merely thinking of a one-liner where a BIDSPath with a sessions suffix is being created :)

[JojoVh]

Dear @sappelhoff and @hoechenberger

Thank you so much for your feedback. It is the about the 5th time I try to make a contribution to Github (Fieldtrip/BIDSvalidator/mne-bids), so I am still eager to learn about this process. Thanks for your remarks.

Concerning the tests that failed, I now adhered to the style standards.
Further, it is not entirely clear to me, where exactly the test dataset is being read from, as it is missing the sessions.tsv indeed
Hence, I wrote a test that wrote session.tsv, and then read and altered it, just the way that was done with the test test_handle_scans_reading for scans.tsv

For any questions or feedback, please let me know
Best wishes
Jonathan

[sappelhoff]

btw: I think the reason why circleCI is not being triggered is due to this setting:

@JonathanVHoecke can you confirm that you don't have a circleci account?

@larsoner why do we have this setting?

more info: https://support.circleci.com/hc/en-us/articles/4413968995099-How-to-set-up-the-Prevent-unregistered-user-spend-feature

[drammock]

why do we have this setting?

because we pay for our CircleCI service, and if a user fails to do basic doc build debugging locally (instead pushing one commit at a time trying to use the CIs to debug) it will use up I think 20ish minutes for each normal doc build, and ~2 hours if they push with [circle full].

[JojoVh]

@sappelhoff. Thank you to get back to me. I confirm I do not have a CircleCI account.

[sappelhoff]

Thanks for the answer @drammock.

@JonathanVHoecke if you want the circleci check to run for your PR (which would be nice), this means that you'd need to:

1. create a circleci account (you can simply log in with your GitHub credentials)
2. always check that your docs build fine locally (see the contributing guide section)
3. take care not to push each commit individually, but rather as coherent groups of commits (in order to save time that the CI is running)

As for updating the tiny_bids dataset, the way to go is to:

1. update this script:

   mne-bids/mne_bids/tests/data/tiny_bids/code/make_tiny_bids_dataset.py

   Line 1 in 9394395

   """Code used to generate the tiny_bids dataset."""

2. run the script with your up-to-date installation of mne-bids
3. commit the resulting changes (confirming that there are no unexpected changes to the tiny_bids dataset)

--> this would be the workflow as opposed to just adding a file like you did here.

I don't really see why the sessions.tsv file was missing in the CI. HOWEVER ...

as @hoechenberger says in his comment above, we only need a much smaller test for the change you propose here.

One potentially good location would be here:

mne-bids/mne_bids/tests/test_path.py

Lines 410 to 423 in 9394395

	# Test behavior of suffix and extension params when suffix and extension
	# are also (not) present in the passed BIDSPath
	bids_path = BIDSPath(
	subject='test', task='task', datatype='eeg', root=tmp_path
	)
	bids_path.mkdir()
	for suffix, extension in zip(
	['eeg', 'eeg', 'events', 'events'],
	['.fif', '.json', '.tsv', '.json']
	):
	bids_path.suffix = suffix
	bids_path.extension = extension
	bids_path.fpath.touch()

As a hint for you, I found this location by:

1. looking for where the tests for BIDSPath are located
2. searching in that file for suffix and checking the different tests for something that looked like it checked suffixes in a general way

Practically:

• please follow my instructions above for getting the circleci check to run (unless you really don't want a circleci account, in which case we'll have to see)
• remove your new test and your other changes that we don't need anymore
• add a tiny tiny test at my suggested location, as recommended by Richard

[drammock]

@sappelhoff note that if you ever need circleCI to run without getting user cooperation you can push an empty commit from an authorized account. I think it's possible to trigger the build manually through the CircleCI interface too.

[hoechenberger]

I just realized that there's a potential problem with the specification of those *_sessions files. A proper BIDS path must contain the entities sub and task. The example provided by @JonathanVHoecke with a path sub-01_sessions.tsv doesn't fit into this scheme. Now, how should we proceed? I don't think splitting *_sessions files by task doesn't make much sense either … so there's an issue in the BIDS spec here, no?

Edit:
Since those files are stored one level up in the folder hierarchy, as compared to the actual recordings, maybe it's okay for them not to bear the task entity.

[sappelhoff]

Since those files are stored one level up in the folder hierarchy, as compared to the actual recordings, maybe it's okay for them not to bear the task entity.

That's how I read it

[hoechenberger]

@sappelhoff I believe this one should be ready for review.

[sappelhoff]

LGTM! Thanks :-)

[welcome]

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

[hoechenberger]

thanks @JonathanVHoecke!