Skip to content

✨Introduced desc-head_bold and changed sbref generating nodeblock #2183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Mar 19, 2025
Merged

✨Introduced desc-head_bold and changed sbref generating nodeblock #2183

merged 7 commits into from
Mar 19, 2025

Conversation

@birajstha
Copy link
Contributor

@birajstha birajstha commented Feb 3, 2025 •
edited
Loading

Fixes

Fixes #2174 by @sgiavasis

Description

Refactors sbref generation by updating BOLD input selection to improve clarity and flexibility. Replaces desc-brain_bold with desc-preproc_bold, introduces desc-head_bold as a bookmark resource, and adds an option to mask sbref in the config.

Technical details

  • Changed the input to desc-preproc_bold
  • Removed the whole-head input desc-head_bold and bold
  • Created desc-head_bold as a bookmark resource at bold_masking nodeblock
  • Added mask_sbref switch in the config as an option.

Tests

Try running the custom pipeline config as mentioned below

registration_workflows:
  functional_registration:
    func_input_prep:
      input: [Selected_Functional_Volume]
functional_preproc:
  func_masking:
    apply_func_mask_in_native_space: off

Screenshots

Ran CPAC test-config with default and abcd-options pre-config. Both completed and the nodeblock connected successfully
image

Furthermore, made a custom pipeline modifying default config with the following changes and the test config ran successfully.

registration_workflows:
  functional_registration:
    func_input_prep:
      input: [Selected_Functional_Volume]
functional_preproc:
  func_masking:
    apply_func_mask_in_native_space: off

Before the fix
image

After the fix, the test-config run completed.
image

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I updated the changelog.
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin 
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@birajstha birajstha linked an issue Feb 3, 2025 that may be closed by this pull request
🔧 Improve coregistration_prep_vol design for choosing whole-head/brain-only #2174
Open
2 tasks
@birajstha birajstha self-assigned this Feb 3, 2025
@birajstha birajstha marked this pull request as ready for review March 6, 2025 21:47
@birajstha birajstha force-pushed the coregistration_prep_vol branch from 233e932 to b07dc13 Compare March 10, 2025 20:32
@birajstha birajstha requested a review from a team March 10, 2025 21:14
shnizzedy
shnizzedy reviewed Mar 13, 2025
View reviewed changes
Copy link
Member

@shnizzedy shnizzedy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this will work as-is, but it's still conceptually unclear to me if

registration_workflows:
  functional_registration:
    coregistration:
      func_input_prep:
        reg_with_skull:

and

registration_workflows:
  functional_registration:
    coregistration:
      func_input_prep:
        mask_sbref:

ever make sense to be different from each other. If not, we can just use reg_with_skull for the switch here

C-PAC/CPAC/registration/registration.py

Lines 3129 to 3135 in 94bd58f

[
"registration_workflows",
"functional_registration",
"coregistration",
"func_input_prep",
"mask_sbref",
],
and don't need to add the new config option.

If we do need the new switch, let's add it to pipeline_config_default.yml too so the comment will copy over to any pipeline config.

@shnizzedy shnizzedy mentioned this pull request Mar 13, 2025
Open
2 tasks
sgiavasis
sgiavasis requested changes Mar 13, 2025
View reviewed changes
Copy link
Collaborator

@sgiavasis sgiavasis left a comment

Choose a reason for hiding this comment

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

Looks good! Just requested two small changes.

@sgiavasis
Copy link
Collaborator

sgiavasis commented Mar 13, 2025

Re: @shnizzedy,

Diving into this deeper - I see where/why this gets confusing. reg_with_skull only comes into play in coregistration_prep_vol, because it is preparing the input to coregistration. So it makes a decision whether to use the skull-stripped bold or not (it takes in the entire timeseries (desc-preproc_bold) and punches out one TR).

Really that logic should be in the coregistration block about which input to use.

In coregistration_prep_mean, it uses desc-mean_bold, which is created before bold masking, so it is whole-head. Which is why the decision was kind of duct-taped into coregistration_prep_vol only.

Then there is coregistration_prep_fmriprep, which creates its own sbref as part of the FSL-AFNI bold masking, and this is an sbref that is already skull-stripped. You can start to see why this confluence of three separate possibilities is so confusing.

In the end I think it's okay if mask_sbref is an option available even if half the time, the sbref will already be skull-stripped at that point. Re-masking won't change anything if someone accidentally leaves it on, the worst case is that it's one unnecessary operation.

The thought arises that we could simply add a masking step in coregistration if people want to ensure the usage of a masked sbref, but in the spirit of keeping things modular, I think it's better as its own nodeblock (what if a user doesn't want to run coregistration, what if they want to output a slate of sbref outputs but wants them masked too?).

I was going to say we should keep reg_with_skull and move it to coregistration, but I see we actually have an input: field under coregistration that never gets used.

I would also say then, let's use that, and decide whether or not to send in brain or whole-head sbref in coregistration. But alas! There would be times when we don't have the whole-head sbref (coregistration_prep_fmriprep) unless we modify that nodeblock to output both versions, and if we also make some conduit for the desc-head_bold that always gets turned into a whole-head sbref but with similar preprocessing options.

Long story short: this is all worth doing, but I think not on the eve of a release!

I think what we need to do:

  • Get rid of reg_with_skull (the one under the coregistration prep part).
  • Remove it from the coregistration_prep_vol nodeblock and simply invoke desc-preproc_bold only. Thus removing the [desc-motion_bold, bold] input option.
  • Keep mask_sbref and make On the default option so the input to coregistration is always the brain-only version by default. If someone really wants a whole-head sbref, they can use coregistration_prep_mean and turn mask_sbref off. The confusing existence of the input: field of coregistration will have no effect on this.
  • Speaking of which, let's remove that input: for now despite the likely possibility it will go right back in pretty soon.
  • Bump a final reworking of this to the next version.

@birajstha birajstha force-pushed the coregistration_prep_vol branch from 709816b to e497d62 Compare March 17, 2025 16:53
@birajstha birajstha requested a review from sgiavasis March 17, 2025 17:40
@sgiavasis sgiavasis merged commit 6a7d31a into develop Mar 19, 2025
24 checks passed
shnizzedy
shnizzedy reviewed Mar 26, 2025
View reviewed changes
@birajstha birajstha mentioned this pull request Mar 27, 2025
Merged
8 tasks
@shnizzedy shnizzedy mentioned this pull request Mar 28, 2025
Open
1 task
@shnizzedy shnizzedy mentioned this pull request Sep 3, 2025
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shnizzedy shnizzedy shnizzedy left review comments

@sgiavasis sgiavasis sgiavasis approved these changes

Assignees

@birajstha birajstha

Labels

None yet

Projects

C-PAC Development
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🔧 Improve coregistration_prep_vol design for choosing whole-head/brain-only

4 participants

@birajstha @sgiavasis @shnizzedy