Skip to content

Conversation

@jan-petr
Copy link
Contributor

@jan-petr jan-petr commented Aug 26, 2022

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:
image

@jan-petr jan-petr self-assigned this Aug 26, 2022
@jan-petr jan-petr linked an issue Sep 7, 2022 that may be closed by this pull request
4 tasks
Copy link
Contributor Author

@jan-petr jan-petr left a 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.

@jan-petr
Copy link
Contributor Author

jan-petr commented Sep 7, 2022

One more question - can we close #1085 as this is already done here?

@jan-petr
Copy link
Contributor Author

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.

@jan-petr jan-petr requested review from HenkMutsaerts and removed request for HenkMutsaerts October 5, 2022 18:40
Copy link
Member

@HenkMutsaerts HenkMutsaerts left a 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');
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes she does

Copy link
Contributor Author

@jan-petr jan-petr left a 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));
Copy link
Contributor Author

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');
Copy link
Contributor Author

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)];
Copy link
Contributor Author

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 = [];
Copy link
Contributor Author

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);
Copy link
Contributor Author

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.

@jan-petr jan-petr force-pushed the feature-#927_FabberQuantificationNew branch from 73abc67 to 203ecc6 Compare October 12, 2022 14:56
Copy link
Member

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

@

Copy link
Member

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

Approved

Copy link
Contributor

@BeatrizPadrela BeatrizPadrela left a 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');
Copy link
Contributor

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 jan-petr force-pushed the feature-#927_FabberQuantificationNew branch from 4ef10ec to 5932913 Compare October 14, 2022 19:27
@jan-petr jan-petr merged commit 5932913 into develop Oct 14, 2022
@jan-petr jan-petr deleted the feature-#927_FabberQuantificationNew branch October 14, 2022 19:28
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.

FSL/FABBER model for multi-TE quantification

4 participants