Skip to content

[SQL/MEG] Add physiological_file.DownloadPath field#10411

Open
MaximeBICMTL wants to merge 1 commit intoaces:mainfrom
MaximeBICMTL:physio_file_download_path
Open

[SQL/MEG] Add physiological_file.DownloadPath field#10411
MaximeBICMTL wants to merge 1 commit intoaces:mainfrom
MaximeBICMTL:physio_file_download_path

Conversation

@MaximeBICMTL
Copy link
Copy Markdown
Contributor

@MaximeBICMTL MaximeBICMTL commented Mar 12, 2026

As discussed on Slack.

Problem

For the EEG data types that LORIS currently handles, the acquisition files are "real" files, which is very nice. However, for the MEG CTF data types, the acquisition files are actually directories. The BIDS specification says that these MEG CTF acquisitions must be stored as directories, not as archives, and the neuroscience libraries like MNE only work on directories. As such, it makes sense to store the MEG CTF as directories in the LORIS BIDS dataset. However, the users must also be able to download these acquisition files, which is problematic if those are directories.

Solution

Split physiological_file.FilePath into two fields FilePath (stays the same) and DownloadPath (new field):

  • For "real" files, like those we currently have with EEG, the values of these fields are identical, as they both point to the same file.
  • For directories, the FilePath field points to the directory in the BIDS dataset, and the DownloadPath points to an archive of that directory.

Whenever computation needs to be done in the dataset, the FilePath field is used, whenever the user wants to download an acquisition file, DownloadPath is used.

Testing

  • ✅ Ran the patch on Raisinread.
  • ✅ Looked for all occurences of physiological_file.FilePath in the electrophysiology_modules.
  • ✅ Downloaded a main EEG acquisition file and its associated files, and confirmed that changing the DownloadPath to an invalid value only breaks the main file download.

@github-actions github-actions Bot added Language: SQL PR or issue that update SQL code Language: PHP PR or issue that update PHP code Module: electrophysiology_browser PR or issue related to electrophysiology_browser module labels Mar 12, 2026
@MaximeBICMTL MaximeBICMTL added Category: Refactor PR or issue that aims to improve the existing code Difficulty: Simple PR or issue that should be easy to implement, review, or test labels Mar 12, 2026
This was referenced Mar 16, 2026
Draft
Merged
@MaximeBICMTL MaximeBICMTL added Category: Feature PR or issue that aims to introduce a new feature Release: SQL patch PR that contains an SQL patch to apply and removed Category: Refactor PR or issue that aims to improve the existing code labels Mar 16, 2026
jeffersoncasimir
jeffersoncasimir requested changes Mar 18, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

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

LGTM, but missing change to raisinbread/RB_files/RB_physiological_file.sql

@github-actions github-actions Bot added the RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset label Mar 19, 2026
@MaximeBICMTL
Copy link
Copy Markdown
Contributor Author

MaximeBICMTL commented Mar 19, 2026

@jeffersoncasimir Added the Raisinbread SQL, good catch!

@MaximeBICMTL MaximeBICMTL force-pushed the physio_file_download_path branch from 10bfac2 to df82158 Compare March 19, 2026 10:32
jeffersoncasimir
jeffersoncasimir approved these changes Mar 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jeffersoncasimir jeffersoncasimir left a comment

Choose a reason for hiding this comment

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

LGTM!

@MaximeBICMTL
Copy link
Copy Markdown
Contributor Author

MaximeBICMTL commented Mar 21, 2026

Merged the main branch to resolve conflict, ready to go!

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Mar 31, 2026

I am not crazy about this because of the possibility of data between the two getting de-synced and not having a single source of truth, and also because of the data duplication.

Is it not possible to either:

  1. Only store the archive, and extract it on the fly when running tools or
  2. Only store the directory, and package it on the fly to download?

@MaximeBICMTL
Copy link
Copy Markdown
Contributor Author

MaximeBICMTL commented Mar 31, 2026 •
edited
Loading

Hhhhm, I see your point but I think it would make the code more complicated and less responsive. IMO storing an archive in the BIDS is a big no-no because that is against the BIDS specification and makes it a lot harder to run BIDS libraries on the dataset, especially dataset-wide analyses where we would have to extract/copy every CTF acquisition in the dataset for each analysis.

Regarding the opposite approach, I think tarring the directory on-request for download takes too much time and is not responsive enough. In fact, we already use the "copy multiple files into an archive for download" approach with the physiological_archive (all acquisition-related files) and physiological_event_archive (all acquisition event files) tables. Maybe we could have the on-demand download archiving but we don't have any infrastructure for that ATM... IMO CTF files are specific enough that this is acceptable.

@MaximeBICMTL MaximeBICMTL force-pushed the physio_file_download_path branch from 9470f37 to 7978223 Compare April 14, 2026 11:23
@MaximeBICMTL
physiological file download path
cae8990 
add raisinbread sql

[SQL/MEG] Add the MEG CTF head shape tables (aces#10412)

The MEG CTF format uses a `headshape.pos` file to register the head
shape of the participant. This file is CTF-specific but BIDS-standard
(the BIDS MEG specification basically allows to use native files of all
specific data types), and is usually shared across all the MEG
acquisitions of a session.

This adds some tables to register this data in the LORIS
physiological database and link it to the relevant MEG files.

The `headshape.pos` file format is unfortunately very sparsely
documented. It is a textual file format whose contents looks like this:

```
85
1		5.60644227	-6.38055462	0.79194871
2		8.06310971	-4.72750212	0.59046631
3		7.58675759	-5.78050209	3.05185917
...
NAS	8.57169649	-0.14219376	-3.33998151
LPA	-0.58286578	7.02263278	0.27529669
RPA	-1.40471662	-7.10183109	0.57320080
```

The first line is the number of points. The coordinates are in
centimeters. It might be useful to use a proper
`physiological_coord_system` in the future, but this can easily be added
later IMO, so I'd like to start by merging this minimal implementation.
@MaximeBICMTL MaximeBICMTL force-pushed the physio_file_download_path branch from 7978223 to cae8990 Compare April 14, 2026 11:30
@MaximeBICMTL
Copy link
Copy Markdown
Contributor Author

MaximeBICMTL commented Apr 14, 2026 •
edited
Loading

Rebased and squashed the commits of this PR to make it easier to work with it locally while waiting to address the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffersoncasimir jeffersoncasimir jeffersoncasimir approved these changes

Assignees

@driusan driusan

Labels

Category: Feature PR or issue that aims to introduce a new feature Difficulty: Simple PR or issue that should be easy to implement, review, or test Language: PHP PR or issue that update PHP code Language: SQL PR or issue that update SQL code Module: electrophysiology_browser PR or issue related to electrophysiology_browser module RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset Release: SQL patch PR that contains an SQL patch to apply

Projects

LORIS MEG
Status: Todo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MaximeBICMTL @driusan @jeffersoncasimir