Skip to content
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.

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

New volume to cifti script #317

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

New volume to cifti script #317

wants to merge 2 commits into from

Conversation

coalsont
Copy link
Member

@coalsont coalsont commented Dec 11, 2024

This adds a new script that should be relatively friendly to map an arbitrary subject-specific volume file (in either MNINonLinear (default) or T1w space) to a standard grayordinates space (default 91282). The similarly-named script in ICAFIX/scripts requires selecting an fMRI name, doesn't use cubic resampling the way fMRIVolume SubcorticalProcessing.sh does, and had other arguments that users probably shouldn't have to mess with (manually specifying fMRI and brainordinate resolution, when the input file and grayordinates choice already define them).

This script currently only takes one input volume file at a time, but takes options, particularly --good-voxels, where it may be desired to have different settings per input anyway. At present its smoothing defaults to 0, which differs from how we run fMRISurface.

However, there is a serious flaw in the current 170494 grayordinates, the Atlas_ROIs volume file is RPI, while the dscalar file next to it uses LPI. The 91282 grayordinates is RPI for both. Should I remake the 170494 dscalar file?

@coalsont coalsont requested review from glasserm and mharms December 11, 2024 00:11
@coalsont
Copy link
Member Author

coalsont commented Dec 11, 2024

One important difference from SubcorticalProcessing.sh is that this script's smoothing and dilation don't operate in "fix zeros" mode, so it gives you the input data as-is without fixing anything other than the subcortical structure overlap with the atlas, and whatever is excluded from the surface by --good-voxels (the subcortical part currently ignores --good-voxels). Since SubcorticalProcessing.sh is part of fMRI specifically, and we want resting state not to contain all-zero locations, it makes sense there, but it isn't clear to me how useful it is for ad-hoc mapping (if the main purpose is visual inspection, it seems like it would be better to see that there is missing data, rather than have it blurred over).

@glasserm
Copy link
Contributor

  1. To fix zeros was an option of the prior script, so it should persist here.
  2. It doesn't look like the PR changes the existing pipeline to use the new code.
  3. I no longer remember why the original code had multiple inputs and outputs (as that is not being used by the pipeline that calls this function). It is probably fine not to support that.

@coalsont
Copy link
Member Author

No, I didn't change any pipelines to use this new script, as it might change their behavior (and I don't have a test setup ready for RecleanClassify).

@mharms
Copy link
Contributor

mharms commented Dec 18, 2024

Could the new script have options such that at some point in the future it could be used in lieu of both ICAFIX/scripts and the relevant code in SubcorticalProcessing.sh, but still generate identical results?

@coalsont
Copy link
Member Author

The ICAFIX/scripts version uses the old gaussian resampling code that we replaced due to the patterns it induces in temporal stdev, and I don't intend to add that to this script, so the outputs wouldn't be identical. I would argue that ideally, RecleanClassify shouldn't be using gaussian resampling for its features anyway, but we'd have to test and/or retrain it.

It wouldn't be advisable to replace just SubcorticalProcessing.sh with this, as that script doesn't do surface mapping. In spirit the new script could replace a majority of fMRISurface, but I wrote this one to specifically not leave behind any intermediates, unlike those scripts, so it wouldn't be compatible in that way.

@mharms
Copy link
Contributor

mharms commented Dec 19, 2024

In that case, sounds like there isn't really anything else to do at this time, other than maybe add a "fix zeros" option, if someone wanted to emulate that behavior through this script?

@glasserm
Copy link
Contributor

We definitely want that option as that is sometimes what we want to do.

@mharms
Copy link
Contributor

mharms commented Dec 19, 2024

Should it generate the "fix zeros" output as a second, additional output? Or, if one wanted to compare the impact of "fix zeros", would one just run it twice, with and without that option? (I'm assuming that everything else is deterministic).

@glasserm
Copy link
Contributor

It should be an option. Normally we use it so not using it is probably the less common case.

@coalsont
Copy link
Member Author

coalsont commented Dec 19, 2024

The ideal place to start fixing zeros is on the input file, so the script should not produce two outputs (there would be a lot of duplicated work from A/B intermediates).

The main reason we fix zeros is to stop correlation from returning nonsense, which is mostly relevant for resting state, so I wouldn't make this generic script default to doing it. There is already dilation to deal with tiny triangles and excluded voxels (and non-overlap with atlas subcortical structures) without conflating those with exact zeros in the input data (so, for instance, mapping binary ROIs to cifti doesn't result in massively increasing their size).

@glasserm
Copy link
Contributor

I am okay with the default not fixing zeros, but we often use it so it needs to be an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants