-
Notifications
You must be signed in to change notification settings - Fork 13
Feature #927 fabber quantification new #1176
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
Conversation
jan-petr
left a comment
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.
Very good!! I found several smaller bugs (that might not work for other sequences) and edited some comments and headers. I have partly fixed them, so I added a comment just so that you understand why I did that and can verify this. And there are also a few comments that still need fixing.
Before merging, we should run UnitTest and TestDataSet to be sure that we haven't messed up the quantification for non-DEBBIE sequences.
|
One more question - can we close #1085 as this is already done here? |
Closing. But please check if Texch maps are correctly exported and if we can generate stats for it. |
HenkMutsaerts
left a comment
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.
Can you please update/clean the issue, so it's easier to read? Now it still has text from Michael Stritt, and it says that the issue is halfway done.
| x.D.RawEPIdir = fullfile(x.D.PopDir, 'Raw_EPI_Check'); | ||
| x.D.T1_ASLREGDIR = fullfile(x.D.PopDir, 'T1_ASLReg'); | ||
| x.D.TTCheckDir = fullfile(x.D.PopDir, 'ATT_Check'); | ||
| x.D.TExchCheckDir = fullfile(x.D.PopDir, 'TExch_Check'); |
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.
Is Texch now defined as the abbreviation we should use? Do Amnah etc use the same abbreviation?
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.
Yes - that was the name from Amnah. Am I right @BeatrizPadrela
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.
yes she does
jan-petr
left a comment
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.
See answers to your commits (I wrote some answers while checking the code, so to make them appear, I have to submit a review :)).
| LDs = LDs(2:end); | ||
| PLD_vector_size = length(unique(x.Q.Initial_PLD)); | ||
| PLDs = PLDs(1:end-(PLD_vector_size/x.Q.TimeEncodedMatrixSize)); % Removing the two longer PLDs - 3.4 and 3.6 | ||
| LDs = LDs(1:length(PLDs)); |
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 second comment was indeed wrong for ordinary multi-PLD, we don't skip anything. I have removed the second comment - we only skip the first volume of a block for Time encoded.
Only the comment was wrong. We don't repeat anything. 063b8d1
| x.D.RawEPIdir = fullfile(x.D.PopDir, 'Raw_EPI_Check'); | ||
| x.D.T1_ASLREGDIR = fullfile(x.D.PopDir, 'T1_ASLReg'); | ||
| x.D.TTCheckDir = fullfile(x.D.PopDir, 'ATT_Check'); | ||
| x.D.TExchCheckDir = fullfile(x.D.PopDir, 'TExch_Check'); |
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.
Yes - that was the name from Amnah. Am I right @BeatrizPadrela
| % Replace CBF with ATT in the output path | ||
| iStringCBF = regexpi(pathOutputCBF, 'CBF'); | ||
| iStringCBF = iStringCBF(end); | ||
| pathOutputTT = [pathOutputCBF(1:(iStringCBF-1)) 'TT' pathOutputCBF((iStringCBF+3):end)]; |
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.
I'm fine with that... Up to you. Currently, we just used the current parameter TT.
| fprintf('%s\n', 'Performing single PLD quantification'); | ||
| [~, CBF] = xASL_quant_SinglePLD(PWI, M0_im, SliceGradient, x, x.Q.bUseBasilQuantification); % also runs BASIL, but only in native space! | ||
| ATT = []; | ||
| TExch = []; |
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.
I've changed that to NaN already in 031559f
| vectorOldOrder = reshape(vectorOldOrder', 1, nEncodedVolumes); | ||
|
|
||
| % Reorder the data | ||
| imEncoded = imEncoded(:,:,:,vectorOldOrder); |
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.
This is not about Walsh/Hadamard. But about switching order of TEs and PLDs.
So we have original (old order) and reordered (new order).
So we have old/new, or we can have reordered/original etc. I don't think that OLD is particularly bad, but can rename to something else.
73abc67 to
203ecc6
Compare
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.
@
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.
Approved
BeatrizPadrela
left a comment
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.
Niceee
| x.D.RawEPIdir = fullfile(x.D.PopDir, 'Raw_EPI_Check'); | ||
| x.D.T1_ASLREGDIR = fullfile(x.D.PopDir, 'T1_ASLReg'); | ||
| x.D.TTCheckDir = fullfile(x.D.PopDir, 'ATT_Check'); | ||
| x.D.TExchCheckDir = fullfile(x.D.PopDir, 'TExch_Check'); |
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.
yes she does
4ef10ec to
5932913
Compare
Linked issue
Closes #927
How to test
I've tested it with BBB_VUmc data (Siemens PCASL 3D GRASE) both with 20 and 64ch coils
Comments
I merged quant_Fabber inside quant_Basil, with if statements for b.modules.asl.bMultiTE, but for the creation of the txt with the options to run FSL, since they were very different, I kept xASL_quant_Basil_Options and xASL_quant_Fabber_Options
the xASL_fsl_RunFSL stayed the same as now I'm using my personal directory with the copy of FSL plus Fabber. This path I add to the dataPar.json as:
