Fixes #639 Hadamard decoding: json fields for Hadamard by BeatrizPadrela · Pull Request #718 · ExploreASL/ExploreASL

BeatrizPadrela · 2021-07-08T13:32:31Z

…eck and adding a comment to xASL_imp_DCM2NII_Subject_ShuffleTheDynamics

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

MichaelStritt · 2021-07-08T14:05:54Z

Hey @BeatrizPadrela & @jan-petr: I know nobody asked for my opinion on this, but I think we should be a bit more thrifty with adding JSON fields. Maybe things like HadamardType or something like that get accepted for ASL-BIDS someday, but things like MultiPLD or MultiTE can basically be determined from other fields, right?

HenkMutsaerts

Great work! Commit title: would be nice to do #639 xASL_bids_BIDSifyASLJSON: add multiPLD' note that if your commit title is too long, it will overflow in the detailed commit message. You can manage this by making your commit message title (first -m) short and the commit message (second -m) longer, e.g. commit -S -m 'short title' -m 'longer detailed message'

Functions/xASL_bids_BIDSifyASLJSON.m

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject_ShuffleTheDynamics.m

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject.m

Modules/xASL_module_ASL.m

MichaelStritt

From my side it would be fine to merge it like this. After merging this to develop we can still check how we're going to handle xASL_module_ASL lines 86-88.

Functions/xASL_bids_BIDSifyASLJSON.m

HenkMutsaerts · 2021-07-29T10:37:16Z

Functions/xASL_bids_BIDSifyASLJSON.m

@@ -192,23 +203,23 @@
 				warning('Did not succeed in repeating PLDs for each TE for Hadamard sequence import');


Note that this warning could be misleading if you have a Hadamard sequence with only one echotime!

Just remove warning? @jan-petr

Functions/xASL_bids_BIDSifyASLJSON.m

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject_ShuffleTheDynamics.m

MichaelStritt · 2021-07-29T15:38:24Z

I think the overall code quality is a lot better now. Please re-test and post the results here @BeatrizPadrela.

MichaelStritt

Minor things that are left to improve. Please check my comments.

Modules/SubModule_Import/xASL_bids_BIDSifyCheckTimeEncoded.m

MichaelStritt

Check out my comment and maybe add that if necessary, otherwise it's fine for me 👍

MichaelStritt · 2021-07-30T14:44:20Z

@BeatrizPadrela: I think the code should work now, but you should really start to test these things more thoroughly. We even have two test datasets for that. Maybe you can start by updating the reference studyPar files of them in the FlavorDatabase? I added this as a task in your bucket. Also try to check out x.logging if there was an error. There all error messages are stored, so you can easily look up in which file and line something goes wrong.

BeatrizPadrela · 2021-07-30T15:04:10Z

@BeatrizPadrela: I think the code should work now, but you should really start to test these things more thoroughly. We even have two test datasets for that. Maybe you can start by updating the reference studyPar files of them in the FlavorDatabase? I added this as a task in your bucket. Also try to check out x.logging if there was an error. There all error messages are stored, so you can easily look up in which file and line something goes wrong.

Yes I can update the studyPar files in the FlavorDataBase for these datasets! And thanks for the x.logging tip :)

…eck and adding a comment to xASL_imp_DCM2NII_Subject_ShuffleTheDynamics

… x struct as input to ShufTheDin

You have to be a bit careful here. The keyword hadamard is also a matlab function, which is why it somehow worked for you, but certainly not as intended. In addition it was theoretically possible that x.modules.asl.bHadamard was true but there was no SeriesDescription and you would have checked that field later without checking for existence first. There was an easy fix for that though. I tested it based on Amnahs Hadamard-4 and it worked perfectly well. One minor warning because of the TotalAcquiredPairs, but maybe we have to see if that would still appear in current develop. I would merge these rather early and then improve it later on.

…rd and in the BIDSify we dont have x struct so isHadamard is enough

CheckIfFME & ReorderTimeEncoded

BeatrizPadrela requested a review from jan-petr July 8, 2021 13:32

MichaelStritt added the feature New feature, enhancement or request label Jul 8, 2021

MichaelStritt assigned amahroo and BeatrizPadrela Jul 8, 2021

jan-petr force-pushed the develop branch from 7a5d3cd to cdc07e1 Compare July 9, 2021 10:42

HenkMutsaerts requested changes Jul 11, 2021

View reviewed changes

Functions/xASL_bids_BIDSifyASLJSON.m Outdated Show resolved Hide resolved

Functions/xASL_bids_BIDSifyASLJSON.m Outdated Show resolved Hide resolved

MichaelStritt self-requested a review July 27, 2021 19:23

MichaelStritt reviewed Jul 27, 2021

View reviewed changes

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject_ShuffleTheDynamics.m Outdated Show resolved Hide resolved

MichaelStritt reviewed Jul 27, 2021

View reviewed changes

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject.m Outdated Show resolved Hide resolved

BeatrizPadrela force-pushed the feature-#639_JsonFieldsHadamard branch 3 times, most recently from a108eae to 574e0f9 Compare July 27, 2021 19:57

MichaelStritt requested review from HenkMutsaerts and MichaelStritt July 27, 2021 20:22

MichaelStritt reviewed Jul 27, 2021

View reviewed changes

Modules/xASL_module_ASL.m Outdated Show resolved Hide resolved

MichaelStritt self-requested a review July 28, 2021 18:57

MichaelStritt approved these changes Jul 28, 2021

View reviewed changes

MichaelStritt linked an issue Jul 28, 2021 that may be closed by this pull request

Hadamard decoding: json fields for Hadamard/multiTE & multiPLD #639

Closed

MichaelStritt changed the title ~~#639 xASL_bids_BIDSifyASLJSON addin multiPLD, multiTE and Hadamard ch…~~ Fixes #639 Hadamard decoding: json fields for Hadamard Jul 28, 2021

HenkMutsaerts requested changes Jul 29, 2021

View reviewed changes

MichaelStritt requested a review from HenkMutsaerts July 29, 2021 15:35

MichaelStritt suggested changes Jul 29, 2021

View reviewed changes

Modules/SubModule_Import/xASL_bids_BIDSifyCheckTimeEncoded.m Outdated Show resolved Hide resolved

Modules/SubModule_Import/xASL_bids_BIDSifyCheckTimeEncoded.m Show resolved Hide resolved

MichaelStritt added the debbie DEBBIE project related issues label Jul 29, 2021

MichaelStritt self-requested a review July 30, 2021 08:37

MichaelStritt reviewed Jul 30, 2021

View reviewed changes

Modules/SubModule_Import/xASL_bids_BIDSifyCheckTimeEncoded.m Outdated Show resolved Hide resolved

MichaelStritt approved these changes Jul 30, 2021

View reviewed changes

BeatrizPadrela and others added 26 commits July 30, 2021 20:18

#639 xASL_bids_BIDSifyASLJSON addin multiPLD, multiTE and Hadamard ch…

4070910

…eck and adding a comment to xASL_imp_DCM2NII_Subject_ShuffleTheDynamics

#639 Changes in ShuffleTheDinamics & BIDSify & DCM2NII_Subject to add…

f8fd44d

… x struct as input to ShufTheDin

#639 xALS_impDCM2NII_...: Do not set bHadamard to false if not FME

e4f6d78

#639 In ShufleThedinamics x.module.asl.bHadamard changes to x.bHadama…

868a4d6

…rd and in the BIDSify we dont have x struct so isHadamard is enough

#639 isHadamard -> bHadamard, only local definition

d2a1025

#639 xASL_bids_BIDSifyASLJSON: str2num fix

69a287b

#639 if bHadamardFME then bHadamard, and other stuff

dbd0ae0

#639 xASL_imp_DCM2NII_Subject_ShuffleTheDynamics: improve code structure

7472364

#639 xASL_imp_DCM2NII_Subject_ShuffleTheDynamics: bTimeEncoded

3017409

#639 xASL_imp_DCM2NII_Subject_ShuffleTheDynamics: use functions

896665c

#639 xASL_imp_DCM2NII_Subject_ShuffleTheDynamics: use functions

ec06000

#639 xASL_bids_BIDSifyASLJSON: bTimeEncoded

41be456

#639 xASL_bids_BIDSifyASLJSON: remove typecasting

eb6fd0f

#639 xASL_bids_BIDSifyASLJSON: use dedicated function

b7c4d56

#639 xASL_bids_BIDSifyCheckTimeEncoded: use dedicated script

cc1071b

#639 xASL_imp_DCM2NII_...: introduce dedicated scripts

5623f17

CheckIfFME & ReorderTimeEncoded

#639 Determine number of total acquired pairs

3120ea5

#639 Finishing xASL_bids_BIDSifyCheckTimeEncoded

013e287

#639 Fix bugs and make code more robust

3947670

#639 xASL_bids_BIDSifyASLJSON: improve warning message

8bf278e

#639 xASL_bids_BIDSifyCheckTimeEncoded: improve header descriptions

ab4f1dc

#639 xASL_bids_BIDSifyCheckTimeEncoded: remove jsonIn output

202e553

#639 xASL_bids_BIDSifyCheckTimeEncoded: numberTEs

c51fc1d

#639 xASL_bids_BIDSifyCheckTimeEncoded: numberTEs

7515e52

#639 xASL_bids_BIDSifyCheckTimeEncoded: numberTIs

397f833

MichaelStritt force-pushed the feature-#639_JsonFieldsHadamard branch from a57d26f to 397f833 Compare July 30, 2021 18:24

MichaelStritt merged commit 397f833 into develop Jul 30, 2021

MichaelStritt deleted the feature-#639_JsonFieldsHadamard branch July 30, 2021 18:26

jan-petr removed the request for review from HenkMutsaerts August 16, 2021 07:49

		@@ -192,23 +203,23 @@
		warning('Did not succeed in repeating PLDs for each TE for Hadamard sequence import');

Conversation

BeatrizPadrela commented Jul 8, 2021

Linked issue

How to test

Comments

Uh oh!

MichaelStritt commented Jul 8, 2021

Uh oh!

HenkMutsaerts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HenkMutsaerts Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

MichaelStritt Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelStritt commented Jul 29, 2021

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelStritt commented Jul 30, 2021

Uh oh!

BeatrizPadrela commented Jul 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants