Skip to content

Conversation

@MaximeBICMTL
Copy link
Contributor

@MaximeBICMTL MaximeBICMTL commented Nov 20, 2025

Description

Make the LORIS EEG chunker into its own Python package. This allows to better isolate this pipeline from the rest of LORIS Python by having it declare its own dependencies and make sure it does not depend on the rest of LORIS Python. It also makes the LORIS EEG chunker installable without the rest of LORIS Python using pip install.

Details

  • Added the loris-eeg-chunker package metadata.
  • Structure the loris-eeg-chunker package using the conventional src-layout.
  • Make sure the main LORIS Python package depends on loris-eeg-chunker.

Testing instructions

If you want to test this PR manually, either use:

  • pip install . in the LORIS-MRI root directory, which will install both LORIS Python along with the LORIS EEG chunker as a dependency.
  • pip install python/loris_eeg_chunker which will only install the LORIS EEG chunker (the main codebase can still rely on environment variables if it is not installed as a package).

Migration

Note that the LORIS EEG chunker scripts have been renamed using the [project.scripts] Python feature (instead of the PATH environment variable):

  • edf_to_chunks.py to edf-to-chunks
  • eeglab_to_chunks.py to eeglab-to-chunks

The LORIS Python scripts that call these subscripts have been updated, so this is only notable if a project calls these scripts directly.

@github-actions github-actions bot added the Language: Python Issue or PR related to the Python codebase label Nov 20, 2025
@MaximeBICMTL MaximeBICMTL added Caveat for Existing Projects Issue or PR that introduces or may introduce breaking changes for existing projects Area: Packaging Issue or PR related to packaging, installation, environment, or configuration Package: EEG chunker PR or issue related to the EEG chunker labels Nov 20, 2025
@MaximeBICMTL MaximeBICMTL force-pushed the eeg_chunker_package branch 2 times, most recently from 8de658f to 7eecdeb Compare November 20, 2025 07:31
@MaximeBICMTL MaximeBICMTL added the Complexity: Simple Issue or PR that should be simple to implement, review, or test label Nov 21, 2025
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.

Tested via bids_import.py. Works seamlessly.

The only issue is that the EEGChunksPath config is not in core, but in an unmerged draft PR.

@MaximeBICMTL
Copy link
Contributor Author

Thanks for the review Jefferson!

I did not know about EEGChunksPath but upon inspection I don't think it is relevant to this PR 🤔. There is a fallback option and that seemed to work well in my tests and #1343:
https://github.com/aces/Loris-MRI/blob/main/python/lib/physiological.py#L1223-L1229

@jeffersoncasimir
Copy link
Contributor

jeffersoncasimir commented Nov 24, 2025

My mistake, the EEGChunksPath problem was not introduced by this PR and I may have confused it with electrophysiology_chunked_dataset_path.

Here is what happened:

  • The chunks were created (in {datasetsDir}/bids_imports/{dataset}_chunks/)
  • The electrophysiology_chunked_dataset_path entries did not get added to physiological_parameter_file
  • I added electrophysiology_chunked_dataset_path entries manually and visualized the recordings

Maybe important detail: I also had #1342 pulled for this test run

@MaximeBICMTL MaximeBICMTL added Complexity: Medium Issue or PR that requires a moderate effort or expertise to implement, review, or test and removed Complexity: Simple Issue or PR that should be simple to implement, review, or test labels Nov 25, 2025
@MaximeBICMTL
Copy link
Contributor Author

Hhhhm, I rebased this PR to benefit from integration tests and I also ran it manually (same thing for #1342), everything is working correctly for me, all the files are created and I can visualize the result in the EEG browser. Let's merge #1342 first and see for this one later.

@MaximeBICMTL
Copy link
Contributor Author

@jeffersoncasimir Can you re-test this PR ? I rebased it on the other merged PRs (integration tests, os.path in EEG) and everything works well in my manual tests, the files are correctly created and I can visualize the recording in the EEG browser.

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.

#1342 (which I approved) changed the expected chunk path. It seems that the os.path replacement must not have been equivalent.

My EEGChunksPath: /data/Loris-MRI/data/bids_imports_chunks/. Chunks ended up in {EEGChunksPath}/sub-PIDCC0821_ses-V03_task-FACE_acq-eeg_eeg.chunks but should have ended up in {EEGChunksPath}/{DatasetName}_chunks/sub-PIDCC0821_ses-V03_task-FACE_acq-eeg_eeg.chunks

My physiological_parameter_file.Value for electrophysiology_chunked_dataset_path ended up as sub-PIDCC0821_ses-V03_task-FACE_acq-eeg_eeg.chunks, but should have been bids_imports/{DatasetName}_chunks/sub-PIDCC0821_ses-V03_task-FACE_acq-eeg_eeg.chunks to match RB examples

So the front-end is looking at the wrong place for the index.json and the chunks are also not saved in the right place

Let me know how your setup looks and ends up, if you think the problem is on my end. The PR is otherwise ready, but it seems like the right time to get the above straightened out.

@MaximeBICMTL
Copy link
Contributor Author

MaximeBICMTL commented Dec 4, 2025

Oooohokay I think I understand better thanks @jeffersoncasimir. In my tests I did not have EEGChunksPath configured and it worked well. But in your case you seem to have it configured and it causes some issues with the code. I will look into this second setup thanks.

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.

Tested with #1345 and works great!

Ready to merge

@MaximeBICMTL MaximeBICMTL added this to the 28.0.0 milestone Dec 22, 2025
@MaximeBICMTL MaximeBICMTL changed the base branch from main to maxime_changes December 22, 2025 09:05
@MaximeBICMTL MaximeBICMTL merged commit bd4d637 into aces:maxime_changes Dec 22, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Packaging Issue or PR related to packaging, installation, environment, or configuration Caveat for Existing Projects Issue or PR that introduces or may introduce breaking changes for existing projects Complexity: Medium Issue or PR that requires a moderate effort or expertise to implement, review, or test Language: Python Issue or PR related to the Python codebase Package: EEG chunker PR or issue related to the EEG chunker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants