Skip to content

Factorize copy BIDS files (PR 1)#1384

Open
MaximeBICMTL wants to merge 4 commits intoaces:bids_staging_branchfrom
MaximeBICMTL:factorize_bids_copy_file
Open

Factorize copy BIDS files (PR 1)#1384
MaximeBICMTL wants to merge 4 commits intoaces:bids_staging_branchfrom
MaximeBICMTL:factorize_bids_copy_file

Conversation

@MaximeBICMTL
Copy link
Contributor

@MaximeBICMTL MaximeBICMTL commented Feb 22, 2026

Extracted from #1335

Description

Factorize the "get BIDS file path in LORIS" and "copy BIDS file to LORIS" operations in the lib.import_bids_dataset.copy_files, and adopt these factorizations in the Eeg class. The goal of this PR is to be able to share the same logic/function across all modalities (MRI / EEG / MEG) once the other PRs land.

Notes

  • There was previously an inheritance parameter to the Eeg.copy_file_to_loris_bids_dir but it was actually never used.
  • I am not sure our rename strategy works in all cases: we rename BIDS files manually, but their might be cross-references to these files across the dataset. In any case, this logic is not worse than the previous one (it's the same logic). Let's factorize first and fix later IMO.

@github-actions github-actions bot added the Language: Python Issue or PR related to the Python codebase label Feb 22, 2026
@MaximeBICMTL MaximeBICMTL changed the base branch from main to bids_staging_branch February 22, 2026 08:39
@MaximeBICMTL MaximeBICMTL force-pushed the factorize_bids_copy_file branch from 534188a to 724c140 Compare February 22, 2026 08:56
@MaximeBICMTL MaximeBICMTL force-pushed the bids_staging_branch branch 2 times, most recently from febe4c2 to 562e26f Compare February 24, 2026 03:52
@MaximeBICMTL MaximeBICMTL force-pushed the factorize_bids_copy_file branch 3 times, most recently from e582a0a to e6e57a7 Compare March 7, 2026 10:13
@MaximeBICMTL MaximeBICMTL changed the title Factorize copy BIDS files Factorize copy BIDS files (PR 1) Mar 12, 2026
@MaximeBICMTL MaximeBICMTL marked this pull request as ready for review March 12, 2026 13:55
@MaximeBICMTL MaximeBICMTL force-pushed the factorize_bids_copy_file branch 2 times, most recently from 9a21c3c to 38ffb61 Compare March 16, 2026 13:04
Copy link
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.

Not sure if it stems from this PR, but a session directory with bids_default_vl was created, but ses-V1 was not injected as an entity in the downstream file names (nor scans.tsv content).

Screenshot 2026-03-18 at 3 22 28 PM

Note: Ran with #1394 patch

@MaximeBICMTL MaximeBICMTL force-pushed the factorize_bids_copy_file branch from 38ffb61 to df77d0a Compare March 19, 2026 07:44
@MaximeBICMTL
Copy link
Contributor Author

Thanks for the catch @jeffersoncasimir !

The first bug with the missing session label for sessionless datasets was introduced in this PR, my bad!
The second bug with the missing session label in scans.tsv in particular seems to already be present on main/27.0.

I added a commit that fixes both of these bugs, and makes the renaming logic more robust in general.
I think there might still be another bug where the name of file in the scans.tsv file is not renamed, but that is out of the scope of this PR IMO. I think the whole scans.tsv logic needs to be refactored (and it will be in a future PR!).

Copy link
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.

Entities are injected in the filenames but the scans.tsv is copied at the wrong level. This file should be copied to the injected session.

Resulting dataset:
Image

Example dataset with 2 sessions:
Image

@MaximeBICMTL
Copy link
Contributor Author

MaximeBICMTL commented Mar 20, 2026

Another good catch of yours @jeffersoncasimir, I looked at the file name but forgot to look at the whole path...

I pushed another commit that fixes this bug. I can confirm that this bug is also present in main/27.0 (basically the copy_scans_tsv_file_to_loris_bids_dir function that I copied from ScansTSV).

Anyway, I'll add it to my list of follow-ups:

  • Add a scans.tsv file in the integration tests.
  • Add a root events.json file in the integration tests (and fix the path concatenation bug).

Copy link
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.

Probably the final issue.

The chunks folder created does not contain the injected default session: sub-ABC0035_task-resting_eeg.chunks, and the chunks are created, but this line triggers the following error:

ERROR: Chunk creation failed, directory '/data/Loris-MRI/data/bids_imports/EEGManyLabs_Resting_State_Dataset_Eimer1996_BIDSVersion_1.9.0_chunks/sub-ABC0035_ses-V1_task-resting_eeg.chunks' does not exist.

@MaximeBICMTL
Copy link
Contributor Author

MaximeBICMTL commented Mar 21, 2026

Okay @jeffersoncasimir, I managed to reproduce the bug. I really should have tested more with the default session label, sorry for that...

Anyway, I can confirm you that the bug is solved by merging #1395. The reason is simple: before #1395, the chunks path was determined from the input file name (which was an oopsie form my part), whereas after it is determined with the LORIS file name.

EDIT: Actually I went ahead and merged #1395 and rebased this branch. I tested a sessionless dataset and it now works as expected.

@MaximeBICMTL MaximeBICMTL force-pushed the factorize_bids_copy_file branch from 63ad0ce to 816096d Compare March 21, 2026 09:16
Copy link
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.

Good to merge!

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

Labels

Language: Python Issue or PR related to the Python codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants