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

Transmit bias pipeline #298

Merged
merged 11 commits into from
Nov 15, 2024
Merged

Transmit bias pipeline #298

merged 11 commits into from
Nov 15, 2024

Conversation

coalsont
Copy link
Member

I will set up compiled matlab before this gets merged. Start with the Examples script to see the overall flow/settings.

Copy link
Contributor

@glasserm glasserm left a comment

Choose a reason for hiding this comment

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

Why did we add a second copy of the 164k meshes to this? We already had these meshes in the pipelines.

@coalsont
Copy link
Member Author

coalsont commented Sep 17, 2024

Why did we add a second copy of the 164k meshes to this? We already had these meshes in the pipelines.

Because the existing names were inconsistent and confusing (and not as easily scriptable). It just happened to be in this PR because that's what I was editing when I decided it was time to finally do something about it.

@coalsont coalsont changed the title initial version of transmit bias pipeline Transmit bias pipeline Sep 18, 2024
@demsarjure
Copy link
Contributor

demsarjure commented Oct 23, 2024

When implementing the interface I noticed a few minor inconsistencies:

  • scanner-grad-coeffs from this pipeline is called gdcoeffs in PreFS.
  • grayordinates-res is called grayordinatesres in PostFS.

@coalsont
Copy link
Member Author

coalsont commented Oct 24, 2024

--grayordinates-res is more readable and is obviously related to --grayordinatesres. I don't know that we expect anyone to copy and paste option flags between any other pipeline and TransmitBias (the batch launcher handles the option names). --gdcoeffs being harder to decipher than --scanner-grad-coeffs is the only reason it wouldn't be obvious to a user that this name means the same thing. If there were inconsistencies between MR FIX and reapply, it would matter because they are closely related, but I don't think it matters here.

@mharms
Copy link
Contributor

mharms commented Oct 24, 2024

It's not just PreFS that uses --gdcoeffs. fMRIVolume, and Diffusion use that as well.
Personally, I think consistency in the flag names is preferred.

Copy link
Contributor

@glasserm glasserm left a comment

Choose a reason for hiding this comment

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

Thanks Tim.

@coalsont
Copy link
Member Author

coalsont commented Nov 9, 2024

I added support in case someone copy-pastes --gdcoeffs or --grayordinatesres from another pipeline. I'd prefer to keep the more readable names in the help info.

@coalsont coalsont merged commit daaeafe into master Nov 15, 2024
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.

4 participants