-
Notifications
You must be signed in to change notification settings - Fork 7
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
BEP016: Initial draft of common filename suffix "model" #46
Conversation
Any opinions on merging this prior to greater broadcast?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging #51, I think that you will need to adjust "parameter" to "param" wherever that appears. I think that it would be good to continue the discussion about the "all" designation, which I highlight in my comment here. Will open a separate discussion, referencing the relevant PRs (this one and #52).
Dimensions of NIfTI image "`sub-01_desc-dti_parameter-all_model.nii.gz`": *I*x*J*x*K*x6 ([parameter vectors](#data-param)) | ||
Dimensions of NIfTI image "`sub-01_desc-dti_parameter-bzero_model.nii.gz`": *I*x*J*x*K* ([scalar](#data-scalar)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the one hand, the first file among these two has an "all" in its file name suggesting that all relevant parameters are found in this file. On the other hand, we immediately have another _model
file right next to it. That's a bit confusing, isn't it? So either the b0 goes into the _all_model
file, or we need to find another way to find all the relevant model parameters distributed across multiple files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But otherwise, I think this is good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an error on my part. "_param-all
" should only appear if it's the only image containing model parameters. If the tensor fitting also yields a b=0 estimate, it needs to be something like "_param-tensor
" and "_param-bzero
".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed exactly this was already described in the prior table. Should be resolved in d3bbb86.
Conflicts: src/05-derivatives/05-diffusion-derivatives.md
We agree to merge this and now we can start with the next |
Draft PR in order to discuss issues described in greater detail in #32.