[MISC] consistently list filename templates; ext --> extension; _photo.jpg --> _photo.<extension>

[sappelhoff]

closes #1456

NOTE: see update below this post

Looking at this more closely, I was a bit surprised to see "channels" as a potential suffix in the filename templates for MOTION ... but not for EEG.

This first commit is to try out what works and what doesn't: adding photo and coordsystem as well (see EEG spec: https://bids-specification.readthedocs.io/en/latest/modality-specific-files/electroencephalography.html)

Maybe we need to discuss this in more depth.

This is how it would look with the present changes: https://bids-specification--1458.org.readthedocs.build/en/1458/modality-specific-files/electroencephalography.html

... it looks good to me, it picks up all the correct extensions.

cc @Remi-Gau

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (4e38968) 87.90% compared to head (9673d88) 87.90%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1458   +/-   ##
=======================================
  Coverage   87.90%   87.90%           
=======================================
  Files          14       14           
  Lines        1273     1273           
=======================================
  Hits         1119     1119           
  Misses        154      154           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[sappelhoff]

update: okay I remembered now that the file templates are listed in the modality sections -- not all at the top:

see e.g., this screenshot:

But that rather means that we'd have to adjust this for NIRS and MOTION ... just so that it's consistent across modalities. WDYT?

I also fixed:

• <ext> --> <extension> for consistency, and
• photo.jpg --> photo.<extension>, because since [ENH] allow for .png and .tif in eeg/ieeg/meg as allowed for micr #1049 we allow other extensions for _photo

[Remi-Gau]

Note (maybe for another PR):

• the filename template for MEG mentions calibration and crosstalk files but there is not a word about them in the text below the filename template. We should somehow add a link to their section in the appendix.

[Remi-Gau]

Can you remove:

• channels
• optodes
• coordsystem
  from the main filename templates of the NIRS given that they have their own section.

[sappelhoff]

@Remi-Gau I think you were looking at an outdated commit, I already addressed your requests in 96eb6d6

[Remi-Gau]

@Remi-Gau I think you were looking at an outdated commit, I already addressed your requests in 96eb6d6

indeed

out of date

[sappelhoff]

the filename template for MEG mentions calibration and crosstalk files but there is not a word about them in the text below the filename template. We should somehow add a link to their section in the appendix.

I added it in e809a9c

[effigies]

Actually, looking at motion, is tracksys required for events.tsv? And is it forbidden for physio/stim.tsv.gz?

[sappelhoff]

cc @sjeung @JuliusWelzel for Chris' question: #1458 (comment)

[Remi-Gau]

the filename template for MEG mentions calibration and crosstalk files but there is not a word about them in the text below the filename template. We should somehow add a link to their section in the appendix.

I added it in e809a9c

Nice !!

adressed changes, thanks for the fix Remi.

[sappelhoff]

Actually, looking at motion, is tracksys required for events.tsv? And is it forbidden for physio/stim.tsv.gz?

The spec text suggests it's optional for events.

bids-specification/src/modality-specific-files/motion.md

Line 66 in 098b88e

Such an events file name SHOULD include the `tracksys` key and looks like `sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv`.

I guess it should be optional for physio/stim as well then 🤔

[Remi-Gau]

Actually, looking at motion, is tracksys required for events.tsv? And is it forbidden for physio/stim.tsv.gz?

The spec text suggests it's optional for events.

bids-specification/src/modality-specific-files/motion.md

Line 66 in 098b88e

Such an events file name SHOULD include the `tracksys` key and looks like `sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv`.

I guess it should be optional for physio/stim as well then thinking

To store events alongside motion data when there are multiple tracking systems simulatenously in use, it is RECOMMENDED to designate a tracking system to the events file. Such an events file name SHOULD include the tracksys key and looks like sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv.

Actually made me notice a couple of things:

• the example in the text does not have the entities in the right order.

It should be:

sub-<label>[_ses-<label>]_task-<label>_tracksys-<label>[_acq-<label>][_run-<index>]_events.tsv

and not

sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv

• if the track-sys is optional for events then the filename template in the text and in the text must be changed to reflect that:

sub-<label>[_ses-<label>]_task-<label>[_tracksys-<label>][_acq-<label>][_run-<index>]_events.tsv

• if the track-sys is required then the filename templates for events are correct but the SHOULD has to be turned into a MUST.

[sappelhoff]

True. I think it'd be good if we could render inline filename templates using the schema as well ... that'd prevent this sort of error

Waiting on determination of required/optional status of tracksys in events/physio/stim

[sjeung]

Hi @sappelhoff @Remi-Gau @effigies

Actually, looking at motion, is tracksys required for events.tsv? And is it forbidden for physio/stim.tsv.gz?

The spec text suggests it's optional for events.

bids-specification/src/modality-specific-files/motion.md

Line 66 in 098b88e

Such an events file name SHOULD include the `tracksys` key and looks like `sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv`.

I guess it should be optional for physio/stim as well then thinking

To store events alongside motion data when there are multiple tracking systems simulatenously in use, it is RECOMMENDED to designate a tracking system to the events file. Such an events file name SHOULD include the tracksys key and looks like sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv.

Actually made me notice a couple of things:

• the example in the text does not have the entities in the right order.

It should be:

sub-<label>[_ses-<label>]_task-<label>_tracksys-<label>[_acq-<label>][_run-<index>]_events.tsv

and not

sub-<label>[_ses-<label>]_task-<label>[_acq-<label>]_tracksys-<label>[_run-<index>]_events.tsv

• if the track-sys is optional for events then the filename template in the text and in the text must be changed to reflect that:

sub-<label>[_ses-<label>]_task-<label>[_tracksys-<label>][_acq-<label>][_run-<index>]_events.tsv

• if the track-sys is required then the filename templates for events are correct but the SHOULD has to be turned into a MUST.

Hi!
Sorry about the somewhat delayed feedback.

First of all, the tracksys entity is optional for both physio and stim, as well as events.

And thanks Remi for spotting the order error, that is certainly a mistake in the text and it has to be updated to

sub-<label>[_ses-<label>]_task-<label>[_tracksys-<label>][_acq-<label>][_run-<index>]_events.tsv

[sappelhoff]

@effigies @Remi-Gau @sjeung @JuliusWelzel from my side, this is now ready for one more review and a potential merge afterwards

[Remi-Gau]

Good to go for me.
thanks @sappelhoff for leading this