Skip to content

[ENH]: Add TemporalSlicer#443

Merged
synchon merged 17 commits intomainfrom
feat/temporal-slicer-preproc
May 9, 2025
Merged

[ENH]: Add TemporalSlicer#443
synchon merged 17 commits intomainfrom
feat/temporal-slicer-preproc

Conversation

@synchon
Copy link
Copy Markdown
Member

@synchon synchon commented Apr 7, 2025

Are you requiring a new dataset or marker?

  • I understand this is not a marker or dataset request

Which feature do you want to include?

To have the option to define which segment of the time series to use for extracting connectomes (define start and end point). For example, cut a 20-minute sequence after 10 minutes and extract connectomes from only the second part.

Use-Case: We have a 24 minute sequence with a task and a control task performed sequentially and want to analyze them separately.

How do you imagine this integrated in junifer?

Cut the time series before calculating the connectomes.

Do you have a sample code that implements this outside of junifer?

Anything else to say?

@kaurao

@jkroell jkroell added enhancement New feature or request triage New issues waiting to be reviewed labels Mar 26, 2025
@synchon
Copy link
Copy Markdown
Member

synchon commented Mar 26, 2025

@jkroell Hi! Thanks for the feature request. Not sure I understand it correctly, do you want an option to perform temporal slicing at the marker level or do you want a preprocessor?

@kaurao
Copy link
Copy Markdown
Collaborator

kaurao commented Mar 26, 2025

I think this can be implemented at different levels, either in preprocessing or in the marker. I guess making it part of preprocessing with make it more general and applicable across different markers?

@synchon
Copy link
Copy Markdown
Member

synchon commented Mar 26, 2025

@kaurao Ok then we go with a new preprocessor.

@synchon synchon added preprocess Issues or pull requests related to preprocessors and removed triage New issues waiting to be reviewed labels Apr 7, 2025
@synchon synchon added this to the 0.0.7 (alpha 6) milestone Apr 7, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2025

Codecov Report

Attention: Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.09%. Comparing base (b9013e7) to head (57acd73).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
junifer/preprocess/_temporal_slicer.py 98.41% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   86.13%   91.09%   +4.96%     
==========================================
  Files         133      134       +1     
  Lines        5617     5422     -195     
  Branches      950      903      -47     
==========================================
+ Hits         4838     4939     +101     
+ Misses        600      309     -291     
+ Partials      179      174       -5     
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 91.09% <98.41%> (+4.96%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
junifer/pipeline/pipeline_component_registry.py 94.64% <ø> (ø)
junifer/preprocess/_temporal_slicer.py 98.41% <98.41%> (ø)

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 7, 2025

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

@synchon synchon changed the title [ENH]: Add feature to cut time series before computing connectomes [ENH]: Add preprocessor to cut time series before computing connectomes Apr 7, 2025
@fraimondo
Copy link
Copy Markdown
Contributor

@synchon @kaurao @jkroell Does it make sense to add the positibility to specify intervals in several ways?

  • [tstart, tend]
  • [tstart, tstart+len]

Ej:

  • 10 minutes starting at 0: tstart=0, len=10*60
  • from minute 10 to minute 20: tstart=10*60, tend=20*60

@fraimondo
Copy link
Copy Markdown
Contributor

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

start and stop, if in seconds, should be floats. If we have sub-second TR we are in trouble. We'll most probably have rounding errors.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 7, 2025

Adding a len doesn't make sense to me as you can calculate that and be sure of your tend depending on t_r. But maybe, as an end-user, Jean's thoughts might differ.

@fraimondo
Copy link
Copy Markdown
Contributor

Adding a len doesn't make sense to me as you can calculate that and be sure of your tend depending on t_r. But maybe, as an end-user, Jean's thoughts might differ.

Why can't we support both?

It's easier to copy/paste a yaml and then change the tstart if you want to analyse 10 minutes from T0 and 10 minutes from T1, where T1 is a number that we know from the acquisition.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 7, 2025

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

start and stop, if in seconds, should be floats. If we have sub-second TR we are in trouble. We'll most probably have rounding errors.

Making them floats is a good idea to be more precise with slicing. I anyway do an int conversion to take care of that, most you get is an IndexError.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 7, 2025

Adding a len doesn't make sense to me as you can calculate that and be sure of your tend depending on t_r. But maybe, as an end-user, Jean's thoughts might differ.

Why can't we support both?

It's easier to copy/paste a yaml and then change the tstart if you want to analyse 10 minutes from T0 and 10 minutes from T1, where T1 is a number that we know from the acquisition.

Of course we can, not a problem. Just wanted to make sure there's a proper use case.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 7, 2025

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

start and stop, if in seconds, should be floats. If we have sub-second TR we are in trouble. We'll most probably have rounding errors.

Making them floats is a good idea to be more precise with slicing. I anyway do an int conversion to take care of that, most you get is an IndexError.

This is taken care of now.

@fraimondo
Copy link
Copy Markdown
Contributor

fraimondo commented Apr 7, 2025

Adding a len doesn't make sense to me as you can calculate that and be sure of your tend depending on t_r. But maybe, as an end-user, Jean's thoughts might differ.

Why can't we support both?
It's easier to copy/paste a yaml and then change the tstart if you want to analyse 10 minutes from T0 and 10 minutes from T1, where T1 is a number that we know from the acquisition.

Of course we can, not a problem. Just wanted to make sure there's a proper use case.

If we can do the math for the user, better.

I had this use case recently because someone merged resting with a task so I had to discard some volumes at the begining. Which also makes me think that in that case I needed:

  • tend=None and len=None should mean until the end of the recording.

Another possible use case:

  • tend<0 could also mean stop end tend seconds from the end. Basically, crop tend seconds from the end.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://juaml.github.io/junifer/pr-preview/pr-443/

Built to branch gh-pages at 2025-04-08 14:49 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@jkroell
Copy link
Copy Markdown
Author

jkroell commented Apr 8, 2025

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

start and stop, if in seconds, should be floats. If we have sub-second TR we are in trouble. We'll most probably have rounding errors.

Making them floats is a good idea to be more precise with slicing. I anyway do an int conversion to take care of that, most you get is an IndexError.

This is taken care of now.

I like the interface! Would it be an option to automatically get the TR, or do you think the user should set that himself?

@fraimondo
Copy link
Copy Markdown
Contributor

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

start and stop, if in seconds, should be floats. If we have sub-second TR we are in trouble. We'll most probably have rounding errors.

Making them floats is a good idea to be more precise with slicing. I anyway do an int conversion to take care of that, most you get is an IndexError.

This is taken care of now.

I like the interface! Would it be an option to automatically get the TR, or do you think the user should set that himself?

If the TR is not specified, it will be taken from the images. However, keep in mind that some software do not take care of the TR properly, so it's always good to have an option to fix it manually.

@jkroell
Copy link
Copy Markdown
Author

jkroell commented Apr 8, 2025

@jkroell How do you like this interface?

preprocess:
  - kind: TemporalSlicer
    start: 0 # int, in second
    stop: 168 # int, in second
    t_r: 2.0  # float, in second (TR)

start and stop, if in seconds, should be floats. If we have sub-second TR we are in trouble. We'll most probably have rounding errors.

Making them floats is a good idea to be more precise with slicing. I anyway do an int conversion to take care of that, most you get is an IndexError.

This is taken care of now.

I like the interface! Would it be an option to automatically get the TR, or do you think the user should set that himself?

If the TR is not specified, it will be taken from the images. However, keep in mind that some software do not take care of the TR properly, so it's always good to have an option to fix it manually.

Okay, makes sense! Looks good to me, then.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 8, 2025

@jkroell Was about to reply, but Fede already gave a nice answer.
@fraimondo Have added support for negative indexing in stop.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 8, 2025

@fraimondo Ok so now we can have stop=None which would take you to end and duration which is added to start to address the situations you described. I believe it's better to adjust the confounds as well?

The interface now looks like:

  • negative stop:
preprocess:
  - kind: TemporalSlicer
    start: 0.0 # float, in second
    stop: -2.0 # -ve float, in second
  • no end slicing:
preprocess:
  - kind: TemporalSlicer
    start: 10.0 # float, in second
    stop: null
  • duration:
preprocess:
  - kind: TemporalSlicer
    start: 10.0 # float, in second; start at 10s
    stop: null
    duration: 20.0 # float, in second; stop at 30s

Does it make sense now? @fraimondo @jkroell @kaurao

@fraimondo
Copy link
Copy Markdown
Contributor

@fraimondo Ok so now we can have stop=None which would take you to end and duration which is added to start to address the situations you described. I believe it's better to adjust the confounds as well?

The interface now looks like:

  • negative stop:
preprocess:
  - kind: TemporalSlicer
    start: 0.0 # float, in second
    stop: -2.0 # -ve float, in second
  • no end slicing:
preprocess:
  - kind: TemporalSlicer
    start: 10.0 # float, in second
    stop: null
  • duration:
preprocess:
  - kind: TemporalSlicer
    start: 10.0 # float, in second; start at 10s
    stop: null
    duration: 20.0 # float, in second; stop at 30s

Does it make sense now? @fraimondo @jkroell @kaurao

Yes! Perfect sense.

And you are right, we need to adjust the confounds so they match. Though I believe this should be used after confound removal.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 10, 2025

And you are right, we need to adjust the confounds so they match. Though I believe this should be used after confound removal.

Ideally it should be after confound removal, but is there a case where one can do it before confound removal or can use the confounds in markers?

@fraimondo
Copy link
Copy Markdown
Contributor

And you are right, we need to adjust the confounds so they match. Though I believe this should be used after confound removal.

Ideally it should be after confound removal, but is there a case where one can do it before confound removal or can use the confounds in markers?

We need to be consistent. So I agree with you. Any picking on the BOLD timeseries should also pick any other data that matches.

@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 10, 2025

And you are right, we need to adjust the confounds so they match. Though I believe this should be used after confound removal.

Ideally it should be after confound removal, but is there a case where one can do it before confound removal or can use the confounds in markers?

We need to be consistent. So I agree with you. Any picking on the BOLD timeseries should also pick any other data that matches.

Okay then I'll make the necessary changes.

@synchon synchon force-pushed the feat/temporal-slicer-preproc branch from 290d50f to 5d020e5 Compare April 11, 2025 07:37
@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 11, 2025

@fraimondo Updated the code to reflect confounds manipulation, I'll request a review, feel free to add more comments before reviewing.

@synchon synchon requested a review from fraimondo April 11, 2025 11:08
Comment thread junifer/preprocess/_temporal_slicer.py
@synchon synchon requested a review from fraimondo April 14, 2025 09:27
@synchon
Copy link
Copy Markdown
Member

synchon commented Apr 25, 2025

@fraimondo Shall we get this in?

@synchon synchon force-pushed the feat/temporal-slicer-preproc branch from 36220d6 to cc3860f Compare April 28, 2025 08:58
@jkroell
Copy link
Copy Markdown
Author

jkroell commented May 9, 2025

Using the slicer after confound removal worked successfully. Thanks for adding this!

@synchon synchon changed the title [ENH]: Add preprocessor to cut time series before computing connectomes [ENH]: Add TemporalSlicer May 9, 2025
@synchon synchon merged commit 9d6458a into main May 9, 2025
20 of 21 checks passed
@synchon synchon deleted the feat/temporal-slicer-preproc branch May 9, 2025 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request preprocess Issues or pull requests related to preprocessors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants