Closes #1044 Revamp import by MichaelStritt · Pull Request #1045 · ExploreASL/ExploreASL

MichaelStritt · 2022-01-06T13:16:18Z

Linked issue

Check out the detailed descriptions in #1044.

Tutorial

Format:

x = ExploreASL_Initialize(DatasetRoot, bImport, bDeface, bProcess, bPause, iWorker, nWorkers)

Defaults:

    defaultDatasetRoot = [];
    defaultImport = [0 0];
    defaultDeface = 0;
    defaultProcess = [0 0 0];
    defaultbPause = 0;
    defaultiWorker = 1;
    defaultnWorkers = 1;

Descriptions:

   DatasetRoot - [DatasetRoot]                 (OPTIONAL, STRING, DEFAULT = [])
   bImport     - [DCM2NII, NII2BIDS]           (OPTIONAL, BOOLEAN, DEFAULT = 0)
   bDeface     - [DEFACE]                      (OPTIONAL, BOOLEAN, DEFAULT = 0)
   bProcess    - [STRUCTURAL, ASL, POPULATION] (OPTIONAL, BOOLEAN, DEFAULT = 0)
   bPause      - [bPause]                      (OPTIONAL, BOOLEAN, DEFAULT = 0)
   iWorker     - [iWorker]                     (OPTIONAL, INTEGER, DEFAULT = 1)
   nWorkers    - [nWorkers]                    (OPTIONAL, INTEGER, DEFAULT = 1)

Testing

Flavor testing

{
   "pathExploreASL": "...\\m.stritt\\Server_xASL\\ExploreASL",
   "pathFlavorDatabase": "...\\m.stritt\\Server_xASL\\FlavorDatabase",
   "cmdCloneFlavors": "git clone [email protected]:ExploreASL/FlavorDatabase.git",
   "flavorList": ["GE_PCASL_3Dspiral_14.0LX_WIP_1", "Philips_PCASL_2DEPI_3.2.1.1_1", "Siemens_PASL_3DGRASE_E11_1"]
}

COMPARISON TABLE:
LOGGING TABLE:

Unit testing

10 Test datasets

ResultsTable =

  11×11 cell array

  Columns 1 through 8

    {'Data'             }    {'mean_qCBF_TotalGM'}    {'median_qCBF_Tot…'}    {'median_qCBF_Dee…'}    {'CoV_qCBF_TotalGM'}    {'GMvol' }    {'WMvol' }    {'CSFvol'}
    {'GE_3Dspiral_T1p…'}    {'65.0938'          }    {'52.5562'          }    {'32.2689'          }    {'0.29578'         }    {[0.6399]}    {[0.5052]}    {[0.4329]}
    {'Philips_2DEPI_F…'}    {'39.1088'          }    {'27.7291'          }    {'6.7084'           }    {'0.71377'         }    {[0.5349]}    {[0.4590]}    {[0.4056]}
    {'Philips_2DEPI_F…'}    {'73.9496'          }    {'56.3649'          }    {'22.9361'          }    {'0.33683'         }    {[0.5442]}    {[0.3921]}    {[0.3564]}
    {'Philips_2DEPI_F…'}    {'48.8956'          }    {'43.3433'          }    {'20.2532'          }    {'0.4192'          }    {[0.6994]}    {[0.5816]}    {[0.2222]}
    {'Philips_2DEPI_L…'}    {'empty'            }    {'14.1723'          }    {'2.9896'           }    {'0.89531'         }    {[0.7099]}    {[0.5136]}    {[0.1939]}
    {'Philips_2DEPI_S…'}    {'45.9448'          }    {'41.156'           }    {'9.4148'           }    {'0.36505'         }    {[0.8167]}    {[0.4596]}    {[0.1516]}
    {'Philips_3DGRASE…'}    {'34.4615'          }    {'25.7101'          }    {'7.604'            }    {'0.65754'         }    {[0.5709]}    {[0.5278]}    {[0.4219]}
    {'Philips_3DGRASE…'}    {'33.0358'          }    {'25.0264'          }    {'9.1691'           }    {'0.43047'         }    {[0.7239]}    {[0.5520]}    {[0.3564]}
    {'Siemens3DGRASE_…'}    {'41.7854'          }    {'42.9831'          }    {'23.3048'          }    {'0.75471'         }    {[0.5191]}    {[0.4658]}    {[0.4054]}
    {'Siemens_2DEPI_n…'}    {'68.6563'          }    {'51.5916'          }    {'13.8922'          }    {'0.32113'         }    {[0.6887]}    {[0.6031]}    {[0.3101]}

  Columns 9 through 11

    {'PipelineCompleted'}    {'TC_ASL_Registra…'}    {'TC_M0_Registrat…'}
    {[                1]}    {[           0.8998]}    {[           0.9321]}
    {[                1]}    {[           0.7296]}    {[           0.8137]}
    {[                1]}    {[           0.9193]}    {[           0.9480]}
    {[                1]}    {[           0.8143]}    {[           0.8344]}
    {[                1]}    {[           0.7593]}    {[           0.8910]}
    {[                1]}    {[           0.8544]}    {[           0.8028]}
    {[                1]}    {[           0.8593]}    {[           0.7547]}
    {[                1]}    {[           0.8477]}    {[           0.8952]}
    {[                1]}    {[           0.7168]}    {[           0.4987]}
    {[                1]}    {[           0.8726]}    {[           0.9589]}

Comments

We probably should re-evaluate if we zip too much now. An easy alternative would be to let xASL_adm_CleanUpX(x) only do the zipping if BIDS2Legacy was done. These things can easily be fixed in a follow-up issue. Maybe we need to update a few tests that I haven't thought about yet, but in general it looks pretty good I'd say. There are more detailed descriptions of the individual tasks in the issue. I think the new ExploreASL format is definitely better and hopefully more user friendly. You can still use the array notation to run DCM2NII and NII2BIDS separately, but most people will just use a 1 there. To the reviewers I want to mention that we can easily add/change things in follow-up issues instead of making this PR more convoluted. Cheers!

MichaelStritt · 2022-01-12T09:42:16Z

@jan-petr: Could you have a look at this PR? I don't think there's that much work left to do, since everything is already tested pretty nicely. Thanks 👍

HenkMutsaerts

Really nice.

The only thing I would not change, is the order of the processing modules.
In the end, each ExploreASL processing module should individually read rawdata and save in derivatives/ExploreASL. Now we still have an interim BIDS2legacy conversion step, which cannot be a separate module, not to confuse the users. So we need to discuss how to deal with this. Any idea?

Perhaps best to run BIDS2legacy at the start of each Structural or ASL module.

Functions/Initialization/xASL_init_printSettings.m

MichaelStritt · 2022-01-17T14:59:40Z

The only thing I would not change, is the order of the processing modules. In the end, each ExploreASL processing module should individually read rawdata and save in derivatives/ExploreASL. Now we still have an interim BIDS2legacy conversion step, which cannot be a separate module, not to confuse the users. So we need to discuss how to deal with this. Any idea?

Perhaps best to run BIDS2legacy at the start of each Structural or ASL module.

I would just write the tutorials accordingly right now. Like in the videos I've shown you, where you just say "run the import", "run the defacing" or "run the processing". I personally always run everything in one go. This is probably a question of what your use-case of ExploreASL looks like, but all in all it's way easier I think. Especially if you get rid of the BIDS2Legacy step later on, you can simply remove that module. If you decide to go the other way of running BIDS2Legacy at the start of each Structural or ASL module, it is still a module, because you don't want to run it again if it ran before, right? What I'm saying is that the way it is right now it makes the most sense to me, but if there's another clever way to code it we can definitely do that. The functionality right now makes sense to me and you don't need to communicate it as a separate module if you don't want to. So there is basically the potential of explaining it to users different than it is implemented, because they don't need to know every little detail of your workflow. I had similar thoughts, but I don't see any other good option to basically hide the separate module.

What you could do is to have three processing modules and run this module every time it wasn't run. So you basically just remove the option to run it manually. This way you hide it pretty well. As a developer I hate that, because it makes it harder to control, but for a user that would probably make sense.

jan-petr

Very good! Some comments in the files. Plus the following:

Update ExploreASL calls (number of processing modules have changed) in the following functions/lines: xASL_ut_module_xASL_module_Population.m:dd, xASL_ut_module_xASL_module_Structural.m:33, xASL_ut_module_xASL_module_ASL.m:33, xASL_ut_function_xASL_bids_BIDS2Legacy.m:38,111,125, xASL_test_Flavors.m:114,xASL_test_Flavors_ExploreASL.m:28, xASL_qc_TestExploreASL.m:336, 340, 343
I guess that one common problem we will have is that people will set processing to 1 but will want to start from Legacy set without having the BIDS data, but they will not know that they should say then [0 1 1 1] - we should make sure that this is then nicely catched and reported - can we add this to UT?

ExploreASL.m

ExploreASL_Initialize.m

ExploreASL.m

ExploreASL_Initialize.m

Functions/Initialization/xASL_init_checkDatasetRoot.m

Testing/Functions/xASL_test_Flavors_ExploreASL.m

Modules/SubModule_Import/xASL_wrp_BIDS2Legacy.m

Functions/Initialization/xASL_init_LoadDataParameterFile.m

jan-petr

Further small thing added.

Modules/SubModule_Import/xASL_wrp_NII2BIDS_Subject.m

MichaelStritt · 2022-01-24T09:14:14Z

Team meeting:

path,1,0 -> only run import (no data loading! no bids2legacy!)
path,0,1 -> bids2legacy & processing
path,0,0 -> bids2legacy & data loading
[],0,0 -> initialize

Edit: we forgot the defacing, so I have to add some more code for that, too.

jan-petr

I fix one error when trying to bLoad data when there wasn't bLoadableData. Should we fix that also on other occurrences?

xASL_adm_CleanUpX.m
xASL_init_checkDatasetRoot.m
xASL_init_DefinePaths.m
xASL_imp_ImportInitialization.m
xASL_imp_DetermineSubjectStructure.m
Probably not needed in all cases?!

HenkMutsaerts · 2022-01-24T17:14:47Z

The only thing I would not change, is the order of the processing modules. In the end, each ExploreASL processing module should individually read rawdata and save in derivatives/ExploreASL. Now we still have an interim BIDS2legacy conversion step, which cannot be a separate module, not to confuse the users. So we need to discuss how to deal with this. Any idea?
Perhaps best to run BIDS2legacy at the start of each Structural or ASL module.

I would just write the tutorials accordingly right now. Like in the videos I've shown you, where you just say "run the import", "run the defacing" or "run the processing". I personally always run everything in one go. This is probably a question of what your use-case of ExploreASL looks like, but all in all it's way easier I think.

I know you, and I have warned you before that this is not the utlimately workflow that was designed for neuroimaging:) The idea by the BIDS guys (and us), is that you have the DICOMs that you convert asap to BIDS rawdata, and then you lock the DICOMs in a vault as sourcedata, and always reuse the BIDS data. So you will (in the future) get BIDS data from others, and give BIDS data to be run in other pipelines. Does this now make more sense? ;)

Especially if you get rid of the BIDS2Legacy step later on, you can simply remove that module. If you decide to go the other way of running BIDS2Legacy at the start of each Structural or ASL module, it is still a module, because you don't want to run it again if it ran before, right?

Well, you would want to rerun it again. The point is that this is a temporary invisible submodule of each module. In the near future, the structural module will load /rawdata/sub-001/ses-001/anat/T1w.nii.gz and output `/derivatives/ExploreASL/sub-001/c1T1.nii'. Therefore, in between we need to copy these files before running each subject, per subject. We could of course delay this improvement, but I would discuss this carefully; if we want to move to 2.0.0 I would really move to the final ExploreASL folder structure, to avoid users seeing us changing the folder structure again in 2.1.0.

What I'm saying is that the way it is right now it makes the most sense to me, but if there's another clever way to code it we can definitely do that. The functionality right now makes sense to me and you don't need to communicate it as a separate module if you don't want to. So there is basically the potential of explaining it to users different than it is implemented, because they don't need to know every little detail of your workflow. I had similar thoughts, but I don't see any other good option to basically hide the separate module.

From your perspective this makes sense. From the bird-eyed perspective of how we want BIDS and ExploreASL to evolve, this differs. When we move to new major versions, I try to push ExploreASL to the final folder layout, which helps all users and mTrial for not having to change stuff.

The point is that ExploreASL is the processing pipeline. And aside from ExploreASL, there is a dicom2BIDS import/conversion tool, that should be independent of ExploreASL. Of course this is not yet the case below the hood, and we can have some intermediate steps before this is reality.

Furthermore, the INPUT of any pipeline like ExploreASL should be rawdata, and the OUTPUT should be derivatives/ExploreASL, ONLY for all subjects/sessions that have been processed. So each module reads the rawdata folder. As an intermediate step, we copy the data from rawdata to derivatives/ExploreASL, but this should be invisible to the user. It should be visible to the user which data have or haven't run.

MichaelStritt · 2022-01-25T11:10:09Z

The code really wasn't ready when all of you started to test again 😉

Most of it should be fixed now. Right now it still runs bids2legacy after defacing, so I'll have to think of a similar solution for that as well. Don't mention modularity 😆 It is modular, but for the implicit things like bids2legacy and data loading we have to create some background if-else statements.

jan-petr · 2022-01-25T15:51:58Z

I would just write the tutorials accordingly right now. Like in the videos I've shown you, where you just say "run the import", "run the defacing" or "run the processing". I personally always run everything in one go. This is probably a question of what your use-case of ExploreASL looks like, but all in all it's way easier I think.

Yes - we need to update the tutorials. A diagram documenting the current workflow in import might be helpful as well.

I know you, and I have warned you before that this is not the utlimately workflow that was designed for neuroimaging:) The idea by the BIDS guys (and us), is that you have the DICOMs that you convert asap to BIDS rawdata, and then you lock the DICOMs in a vault as sourcedata, and always reuse the BIDS data. So you will (in the future) get BIDS data from others, and give BIDS data to be run in other pipelines. Does this now make more sense? ;)

As discussed over whatsapp - that's the ultimate goal, but a lot of stuff needs to be done prior to that!

Well, you would want to rerun it again. The point is that this is a temporary invisible submodule of each module. In the near future, the structural module will load /rawdata/sub-001/ses-001/anat/T1w.nii.gz and output `/derivatives/ExploreASL/sub-001/c1T1.nii'. Therefore, in between we need to copy these files before running each subject, per subject. We could of course delay this improvement, but I would discuss this carefully; if we want to move to 2.0.0 I would really move to the final ExploreASL folder structure, to avoid users seeing us changing the folder structure again in 2.1.0.

Yes - the ideal goal, added as issue #1066 - but several steps need to be done before we start implementing this - see issue #165.

The point is that ExploreASL is the processing pipeline. And aside from ExploreASL, there is a dicom2BIDS import/conversion tool, that should be independent of ExploreASL. Of course this is not yet the case below the hood, and we can have some intermediate steps before this is reality.

Yes. We are now making the first step in separating those.

Furthermore, the INPUT of any pipeline like ExploreASL should be rawdata, and the OUTPUT should be derivatives/ExploreASL, ONLY for all subjects/sessions that have been processed. So each module reads the rawdata folder. As an intermediate step, we copy the data from rawdata to derivatives/ExploreASL, but this should be invisible to the user. It should be visible to the user which data have or haven't run.

Currently - this is already invisible. We still copy stuff to derivatives, but it is invisible. The user doesn't have to take care of it. And it is done only once and only for files that haven't been copies from rawdata yet. If you add new subjects, then you simply re-run the pipeline and it will do BIDS2Legacy only for these new files. So this already works in the way we want it (except for still making the copy - but that's a long-run issue).

MichaelStritt · 2022-01-25T15:59:10Z

@jan-petr: Check out my test results in the original PR message 😄

jan-petr · 2022-01-25T16:02:14Z

f making this PR more co

UTs look good. TestDataSets also look good. We just wanted to be sure that these run and they do ;) So that's fine.

I am now checking the Flavors-import.

jan-petr

All good now

HenkMutsaerts · 2022-01-26T06:53:18Z

Let's discuss this, best would be to keep this as "sub-module" within the structural or ASL module.

The point is that ExploreASL is the processing pipeline. And aside from ExploreASL, there is a dicom2BIDS conversion tool, that is completely independent of ExploreASL. Of course this is not yet the case below the hood, and we can have some intermediate steps before this is reality.

Furthermore, the INPUT of any pipeline like ExploreASL should be rawdata, and the OUTPUT should be derivatives/ExploreASL, ONLY for all subjects/sessions that have been processed. So each module reads the rawdata folder. As an intermediate step, we copy the data from rawdata to derivatives/ExploreASL, but this should be invisible to the user. It should be visible to the user which data have or haven't run.

Let's save this for 2.0.0 (not for the current 1.10.0 :)

HenkMutsaerts

When I run ExploreASL('TestDataSet'); great feedback to the screen! Only few confusing points:

Empty session number, setting session number to 1...

This feedback should contain the subject name so the user know which one the change is applied to. Also, I would remove this warning/text at all if there is only a single session.

We get feedback that we're missing the dataPar.json and that a default one is created. This is fine for bids2legacy but when we only load data, we don't process so shouldn't load the dataPar.json?

Overwriting x.dir.dataPar... -> this text is unnecessary

When we run ExploreASL('TestDataSet',1);but thesourcedata` folder doesn't exist, this should give a single error and then skip data loading (or load ExploreASL only). Now it gives a warning and still tries to load the data, giving further errors confusing to the user.
When ExploreASL processing has crashed, the next time you run ExploreASL, bids2legacy starts zipping the temporary files that were left from the previous processing. Why does bids2legacy even zip?
Note that ExploreASL('TestDataSet'); only loads the data, so in the near future this won't copy data (the data copying is a temporary "pre-step" for when processing is run. The fact that we now temporarily run bids2legacy when only loading data is a temporary "quick and dirty" solution so I wouldn't complicate this and simply never zip in the bids2legacy module
Please delete the full deface module, this is confusing. The bDeface is something that we/the community haven't decided upon if this should be in the import or process. If you do the latter, the order stays the same so it doesn't confuse the users (remind that we want backward compatibility as much as possible). So let's remove bDeface fully to the user: probably best to comment these parts out, and not mention it in the printed text/warnings.
BTW: please undo the folder ExploreASL/Templates, we use the word "Templates" a lot for images/atlases, so that would be confusing.

Functions/Administration/xASL_adm_CleanUpX.m

…update

@jan-petr

… line @jan-petr: I do not get how this worked before. The visualization settings and some other paths are definitely needed in the general dataset independent initialization, otherwise the 10 test dataset tests crash, because they need the wb masks. I moved some calls from the data dependent to the data independent settings to make this work again. Or am I thinking wrong? Cheers!

Note that this is not required for xASL_init_PrintUserFeedback, as the x fields used by the latter will not change by ExploreASL_Import

MichaelStritt self-assigned this Jan 6, 2022

MichaelStritt added the revamp Restructuring of ExploreASL label Jan 6, 2022

MichaelStritt linked an issue Jan 6, 2022 that may be closed by this pull request

Import revamp of wrappers and IO #1044

Closed

11 tasks

MichaelStritt requested review from HenkMutsaerts and jan-petr January 6, 2022 13:25

HenkMutsaerts requested changes Jan 17, 2022

View reviewed changes

Functions/Initialization/xASL_init_printSettings.m Outdated Show resolved Hide resolved

MichaelStritt requested a review from HenkMutsaerts January 17, 2022 15:03

jan-petr requested changes Jan 18, 2022

View reviewed changes

Modules/SubModule_Import/xASL_wrp_NII2BIDS_Subject.m Show resolved Hide resolved

This comment has been minimized.

Sign in to view

MichaelStritt requested a review from jan-petr January 21, 2022 16:45

jan-petr requested changes Jan 24, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

MichaelStritt force-pushed the revamp-#1044_Import branch from 28dfe4e to a65e597 Compare January 25, 2022 11:07

MichaelStritt requested a review from jan-petr January 25, 2022 15:09

jan-petr approved these changes Jan 25, 2022

View reviewed changes

jan-petr requested review from HenkMutsaerts and removed request for HenkMutsaerts January 25, 2022 17:49

HenkMutsaerts requested changes Jan 26, 2022

View reviewed changes

Functions/Administration/xASL_adm_CleanUpX.m Show resolved Hide resolved

MichaelStritt and others added 27 commits April 28, 2022 19:14

#1044 Review general: detect structural problems in testing and note …

ca85cc3

…update

#1044 Review general: fix scripts/testing to work with derivatives only

339c59b

#1044 Review general: fix spm testing paths

8e03735

#1044 Review general: update test prefix

2ce94cd

#1044 Review general: fix population path

b28e678

#1044 Review general: update date

a2ae47d

#1044 Second review: Add warning for old import modules format

8042100

#1044 Second review: Rename ImportModules to bImport

98c78da

#1044 Second review: Rename ProcessModules to bProcess

374fb18

#1044 Second review: fix data loading issues

2137160

#1044 Second review: improve testing regarding deface/bids2legacy

d71fef4

#1044 Second review: modularity of defacing module

157ac92

#1044 Second review: minor test script fix

6ab848b

#1044 Zip only derivatives and rawdata not all

5e9ae11

#1044 Separate Import and Process init

7f0ed81

#1044 Further separation of BIDS2Legacy and Import

0602938

#1044 Use imPar inside x-struct

1a96147

#1044 Minor edits

295dcf8

#1044 Revert Deface as a separate module

d03c3ce

#1044 Update tests for Deface

c941bde

#1044 Only a single xASL_init_SubStructs

b3d65ad

#1044 Minor bugfix

015fd93

#1044 ExploreASL_Initialize: Minor bugfix

91b2b78

#1044 xASL_num2str: Minor fix

632c1d8

#1044 Master_UT fix DRO call

4883091

#1044 ExploreASL: do not pass import x through

c6c7639

Note that this is not required for xASL_init_PrintUserFeedback, as the x fields used by the latter will not change by ExploreASL_Import

jan-petr force-pushed the revamp-#1044_Import branch from 8ddbaab to c6c7639 Compare April 28, 2022 17:15

jan-petr merged commit c6c7639 into develop Apr 28, 2022

jan-petr deleted the revamp-#1044_Import branch April 28, 2022 17:16

Conversation

MichaelStritt commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked issue

Tutorial

Testing

Flavor testing

Unit testing

10 Test datasets

Comments

Uh oh!

MichaelStritt commented Jan 12, 2022

Uh oh!

HenkMutsaerts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelStritt commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

MichaelStritt commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

HenkMutsaerts commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelStritt commented Jan 25, 2022

Uh oh!

jan-petr commented Jan 25, 2022

Uh oh!

MichaelStritt commented Jan 25, 2022

Uh oh!

jan-petr commented Jan 25, 2022

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

HenkMutsaerts commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HenkMutsaerts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichaelStritt commented Jan 6, 2022 •

edited

Loading

MichaelStritt commented Jan 17, 2022 •

edited

Loading

MichaelStritt commented Jan 24, 2022 •

edited

Loading

HenkMutsaerts commented Jan 24, 2022 •

edited

Loading

HenkMutsaerts commented Jan 26, 2022 •

edited

Loading