Skip to content

Conversation

@birajstha
Copy link
Contributor

@birajstha birajstha commented Feb 3, 2025

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

@shnizzedy shnizzedy left a comment

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

[
"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.

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

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

4 participants