Feature #640 hadamard decoding by BeatrizPadrela · Pull Request #840 · ExploreASL/ExploreASL

BeatrizPadrela · 2021-09-15T13:35:52Z

Linked issue

Required: mention the issue number of the linked feature or bug here

How to test

Required: if not defined in the linked issue, add a simple test description here

Comments

Optional: add helpful comments for the reviewers here

BeatrizPadrela · 2021-09-16T14:28:25Z

Ok I checked everything with Amnah's data and it creates these nice Volumes (with Hadamard4, so we only have 3 TIs) and actually two repetitions. 3TI * 8TEs * 2Rep = 48 volumes

However, at the end of the xASL_wrp_ResampleASL, there's this part where some averages are made, and of course it doesn't work for Hadamard. Should we skip it or just adapt it?

BeatrizPadrela · 2021-09-16T14:29:12Z

Moreover, I added an output to the decoding function (ASL_im) because we need to save it for both Standard and MNI space (see lin 238)

In red we have the TimeEncoded part and in blue we have the normal data procedure.
the normal procedure includes saving a PWI4D=ASL_im, and a PWI which is a mean of the ASL_im volumes
To the TimeEncoded part I only aded the PWI4D=ASL_im. Should I also add the PWI? I think we will actually not use it afterwards for TimeEncoded data

MichaelStritt · 2021-09-16T14:37:59Z

@BeatrizPadrela & @jan-petr

However, at the end of the xASL_wrp_ResampleASL, there's this part where some averages are made, and of course it doesn't work for Hadamard. Should we skip it or just adapt it?

I mean, I know how to fix these things, but you guys probably need to agree on how these things should be fixed first.

In red we have the TimeEncoded part and in blue we have the normal data procedure. The normal procedure includes saving a PWI4D=ASL_im, and a PWI which is a mean of the ASL_im volumes. To the TimeEncoded part I only added the PWI4D=ASL_im. Should I also add the PWI? I think we will actually not use it afterwards for TimeEncoded data.

Again, we probably need the expertise from @jan-petr on this.

Modules/SubModule_ASL/xASL_wrp_ResampleASL.m

HenkMutsaerts

Nice work! Some questions, most comments are on redesigning your new function so it becomes readable and understandable, take a look at other existing functions as a template (e.g., xASL_system that I just wrote in #845).

HenkMutsaerts · 2021-09-18T10:57:45Z

Functions/xASL_bids_BIDSifyCheckTimeEncoded.m

            numberTIs = xASL_str2num(jsonOut.SeriesDescription(startDetails+3:startDetails+4));
            numberTEs = xASL_str2num(jsonOut.SeriesDescription(startDetails+10:startDetails+11));
            fprintf('FME sequence, Hadamard-%d encoded images, %d TIs, %d TEs\n', jsonOut.TimeEncodedMatrixSize, numberTIs, numberTEs);
+            jsonOut.TimeEncodedEchoTimes = numberTEs;


@MichaelStritt @jan-petr does it make sense to replace these variable names with BIDS field names, so these scripts are easier to debug?

So just...

timEncodedInversionTimes = xASL_str2num(jsonOut.SeriesDescription(startDetails+3:startDetails+4)); timEncodedEchoTimes = xASL_str2num(jsonOut.SeriesDescription(startDetails+10:startDetails+11)); jsonOut.TimeEncodedEchoTimes = timEncodedEchoTimes;

...?

Sure, we can, don't see a huge benefit, but it's easy to do.

HenkMutsaerts · 2021-09-18T10:59:21Z

Functions/xASL_quant_HadamardDecoding.m

+function [imASL_reorder] = xASL_quant_HadamardDecoding(Encoded_ASL, xDecodingFields, NumberEchoTimes)
+%xASL_im_HadamardDecoding Hadamard-4 & Hadamard-8 Decoding
+%
+% FORMAT:       [Decoded_ASL] = xASL_im_HadamardDecoding(HadamardType, sec)


@BeatrizPadrela this doesn't fit with the function line above?
what does sec mean? It says here that it is an output from another function, but it would be nicer to have a longer variable name that is directly interpretable

@BeatrizPadrela:

just change it to this:

function [imASL_reorder] = xASL_quant_HadamardDecoding(Encoded_ASL, xDecodingFields, NumberEchoTimes) %xASL_quant_HadamardDecodingHadamard-4 & Hadamard-8 Decoding % % FORMAT: [imASL_reorder] = xASL_quant_HadamardDecoding(Encoded_ASL, xDecodingFields, NumberEchoTimes)

HenkMutsaerts · 2021-09-18T11:00:14Z

Functions/xASL_quant_HadamardDecoding.m

+
+% Decoding Fields
+
+TimeEncodedMatrixType = xDecodingFields.TimeEncodedMatrixType;


What happens if this field doesn't exist? :)

Same for the other fields. You can check them, and give warnings if they are missing. This should of course not happen, but if it happens for whatever reason — e.g., there is something going wrong somewhere else in another function — then you are notified nicely, which can save you time in debugging later.

Also, an input field check is lacking, like:

if margin<3 || isempty('variable name')

Check other functions on how this should be done. Also, in the header you should note if an input is REQUIRED or OPTIONAL, and if OPTIONAL, what the default value is. See also in other functions.

Valid point, we never really know if those exist if the data is a bit different. We could optimize this in a second PR where we also support the data of Koen though, because there those things will probably "break".

I'd suggest just to add some...

if nargin ... if isfield(...) etc.

... for now and then we improve the logic within the Koen PR.

HenkMutsaerts · 2021-09-18T11:03:36Z

Functions/xASL_quant_HadamardDecoding.m

+    DecodingMatrix_input = [];
+end
+
+% #### For Walsh Decoding Matrix ####


Also here, ### is the same as %%% but for a non-Matlab language ;)

Please check xASL_system which I just created as a template, also for splitting the function in steps that are then easy to read, and that are listed in the function header.

Yeah, but again, xASL_system was created in the meantime. Let's improve those in the next PR with Koens data, then we don't have to do that much cherry-picking and so on.

HenkMutsaerts · 2021-09-18T11:07:43Z

Functions/xASL_quant_HadamardDecoding.m

+    end
+
+    % #### For Hadamard Decoding Matrix ####
+elseif strcmp(TimeEncodedMatrixType,'Hadamard')


what happens if this field is not Hadamard or Walsh? Then there should be a warning right?
Also, you can use strcmpi to make your life easier. If you then accidentally typed hadamard instead of Hadamard it will still work.

Let's do it 100% correctly like this:

switch lower(TimeEncodedMatrixType) case 'walsh' ... case 'hadamard' ... otherwise ... end

HenkMutsaerts · 2021-09-18T11:27:21Z

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject_ShuffleTheDynamics.m

    if xASL_exist(x.dir.studyPar,'file')
        studyPar = spm_jsonread(x.dir.studyPar);
-        if isfield(studyPar,'TimeEncodedMatrixSize') && ~isempty(studyPar,'TimeEncodedMatrixSize') || ... % Should be 4, 8 or 12
+        if isfield(studyPar,'TimeEncodedMatrixSize') && ~isempty(studyPar.TimeEncodedMatrixSize) || ... % Should be 4, 8 or 12


Do we need a warning or feedback here?

Let's not change this too much, because it was already modified/renamed etc. in another PR. Keep it like it is for now 👍

HenkMutsaerts · 2021-09-18T11:27:48Z

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject_ShuffleTheDynamics.m

@@ -159,7 +159,7 @@

    if xASL_exist(x.dir.studyPar,'file')


For a json file, exist works as well as xASL_exist

Let's not change this too much, because it was already modified/renamed etc. in another PR. Keep it like it is for now 👍

HenkMutsaerts · 2021-09-18T11:28:44Z

Modules/xASL_module_ASL.m

+%% TimeEncoded parsing
+
+% Check if TimeEncoded is defined
+if isfield(x,'TimeEncodedMatrixType') || ~isempty(x.TimeEncodedMatrixType)


Should this be || or && / :) Same below!

Probably &&, right?

yes of course sorry, done

HenkMutsaerts · 2021-09-18T11:29:50Z

Modules/SubModule_ASL/xASL_wrp_ResampleASL.m

-    else    
+
+        % Decoding of TimeEncoded data ===================
+    elseif x.modules.asl.bTimeEncoded 


Is this the correct position for this function? So it occurs after motion correction, but before quantification? Should the function then be called quant or is it rather im?

Maybe im is better 👍

HenkMutsaerts · 2021-09-18T11:30:12Z

Modules/SubModule_ASL/xASL_wrp_ResampleASL.m

@@ -222,7 +227,18 @@ function xASL_wrp_ResampleASL(x)
    elseif round(dim4/2)~=dim4/2


Can this be the case for Hadamard or Walsh?

So I guess @amahroo is checking for uneven numbers here 🤔

I didn't make any changes in the resample function, no idea :/

Ahhh true, no one changed the code here. Is this one of your "teaching" questions @HenkMutsaerts ?

since we have multiples of 4 and 8, this should not be the case for Hadamard/Walsh

MichaelStritt · 2021-09-20T11:52:59Z

Functions/xASL_quant_HadamardDecoding.m

+if nargin<3 && isempty(Encoded_ASL)
+    warning('Encoded_ASL input is empty');
+end
+
+if nargin<3 && isempty(xDecodingFields)
+    warning('xDecodingFields input is empty');
 end

+if nargin<3 && isempty(NumberEchoTimes)
+    warning('NumberEchoTimes input is empty');
+end


@BeatrizPadrela I think I'm distracting you too much 😆 try nargin<1, nargin<2, ... 😉

… with Amnahs code

…ons for TimeEncoded data

…e is saved

…eEncoded data

BeatrizPadrela requested review from MichaelStritt and jan-petr September 15, 2021 13:35

MichaelStritt added the feature New feature, enhancement or request label Sep 15, 2021

MichaelStritt assigned BeatrizPadrela Sep 15, 2021

BeatrizPadrela linked an issue Sep 15, 2021 that may be closed by this pull request

Hadamard decoding: calculate the new PWI.nii #640

Closed

BeatrizPadrela added this to the Release 1.9.0 milestone Sep 15, 2021

MichaelStritt removed this from the Release 1.9.0 milestone Sep 16, 2021

BeatrizPadrela requested a review from HenkMutsaerts September 16, 2021 14:33

MichaelStritt reviewed Sep 16, 2021

View reviewed changes

Modules/SubModule_ASL/xASL_wrp_ResampleASL.m Outdated Show resolved Hide resolved

HenkMutsaerts requested changes Sep 18, 2021

View reviewed changes

MichaelStritt requested review from HenkMutsaerts, MichaelStritt and amahroo September 20, 2021 11:34

MichaelStritt reviewed Sep 20, 2021

View reviewed changes

BeatrizPadrela added 11 commits September 20, 2021 14:42

#640 xASL_module_ASL recognizes TimeEncoded, xASL_im_HadamardDecoding…

b32ca07

… with Amnahs code

#640 Finalising HadamardDecoding function and improving other functi…

849b0bf

…ons for TimeEncoded data

#640 Deleting output of quant_HadamardDecoding since the decoded imag…

fea6edc

…e is saved

#640 Finishing Hadamard decoding function

b1b02b9

#640 xASL_wrp_ResampleASL - Mean Control Image: Adding option for Tim…

bce1d5e

…eEncoded data

#640 adding Hadamard matrices + comments

8a43233

#640 HadamardDecoding:normalization factor,ResampleASL:saving PWI4D

44c669b

#640 Hadamard changes related to Henk s review

3c7ed7e

#640 Just adding empty input warnings

f929366

#640 Input warnings

0a6e881

#640 Input warningsss

188554d

MichaelStritt force-pushed the feature-#640_HadamardDecoding branch from 7979b73 to 188554d Compare September 20, 2021 12:45

MichaelStritt merged commit 188554d into develop Sep 20, 2021

MichaelStritt deleted the feature-#640_HadamardDecoding branch September 20, 2021 12:46


		% Decoding Fields

		TimeEncodedMatrixType = xDecodingFields.TimeEncodedMatrixType;

		@@ -222,7 +227,18 @@ function xASL_wrp_ResampleASL(x)
		elseif round(dim4/2)~=dim4/2

Conversation

BeatrizPadrela commented Sep 15, 2021

Linked issue

How to test

Comments

Uh oh!

BeatrizPadrela commented Sep 16, 2021

Uh oh!

BeatrizPadrela commented Sep 16, 2021

Uh oh!

MichaelStritt commented Sep 16, 2021

Uh oh!

Uh oh!

HenkMutsaerts left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeatrizPadrela Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeatrizPadrela Sep 20, 2021 •

edited

Loading