Feature #588 Manage ExploreASL input path #596

MichaelStritt · 2021-05-22T14:20:17Z

Linked issue

Check out #588

How to test

Test description in #588

Comments

We had a meeting about this issue. A lot of the descriptions are in #588. Would be helpful if somebody besides me would also test the behavior.

MichaelStritt · 2021-05-22T15:25:57Z

Major changes

Check out the Files changed tab.

ExploreASL_UpdateStructure: rename DataParPath to StudyRoot
ExploreASL: rename DataParPath to StudyRoot
ExploreASL_ImportMaster: Add dir field to x, add opts field to x, check x.opts.StudyRoot, add try catch statements and logging
ExploreASL_Initialize: rename DataParPath to StudyRoot, move boolean checks to ExploreASL_Initialize_GetBooleansImportProcess, add dir field to x, add opts field to x, improve ExploreASL directory search, move check for StudyRoot to ExploreASL_Initialize_checkStudyRoot (here most of the new functionality happens), move basic print statements to ExploreASL_Initialize_basicFeedback, rename ExploreASL_Initialize_printSettings to xASL_init_printSettings and move it to a separate script, do the same for xASL_init_Toolboxes, xASL_init_RemoveLockDirs, xASL_init_DefinePaths, xASL_init_LoadDataParameterFile, ... , use ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 for the outdated calls of ExploreASL (using JSON files)
ExploreASL_Master: rename DataParPath to StudyRoot
xASL_bids_BIDS2Legacy: use new x structure fields
...
xASL_imp_CreateSummaryFile: in this and some other files we had to change AnalysisRoot to TempRoot, since we rename the analysis folder to temp
...
xASL_test_IntegrationTesting: my ideas for the integration testing (full bids pipeline test using ExploreASL_Master Run TestFlavors using ExploreASL_Master #583 )
xASL_test_UnitTesting: renamed

jan-petr

I went through everything and looks good. I will also try some testing. In the meantime. Can you please check this question? Probably OK, just asking to be sure... "All JSONs in Development/ConfigFiles - rename AnalysisRoot to TempRoot?? Or we probably keep AnalysisRoot there for the old Import, right Michael?"

MichaelStritt · 2021-05-25T19:11:52Z

I went through everything and looks good. I will also try some testing. In the meantime. Can you please check this question? Probably OK, just asking to be sure... "All JSONs in Development/ConfigFiles - rename AnalysisRoot to TempRoot?? Or we probably keep AnalysisRoot there for the old Import, right Michael?"

Oh damn, good that you thought of that. A lot of the changes we do make sense when talking about them, but it's really easy to forget these things for backwards compatibility. We should really move ExploreASL_ImportConfig.m to Developement with v1.8.0, so that people stop using it.

For now it shouldn't be a problem if the ConfigFiles contain both AnalysisRoot and TempRoot 4024cb9.

jan-petr

If you run ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/sourceStructure.json',[0 1 0 1],0) from the flavors, it should report that 'temp' is missing and shouldn't continue
ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/studyPar.json',[1 0 0 0],0) reports no matching JSON found. Which is funny, because the JSON is there, it's a wrong JSON though - shouldn't it report something else?
ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/sourceStructure.json',[0 0 0 1],0) - this works, but shouldn't it report that sourceStructure is incorrect and studyPar.json was used?
ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/studyPar.json',[0 0 0 1],0) - why this crashes? Incorrect JSON, but the other calls survive this...
ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/derivatives',[0 0 0 0],1) This doesn't do anything - though if you remove 'derivatives' or add 'ExploreASL' it does run - perhaps not a good idea to look one directory up, but if it spots that it's in derivatives, it can check for ExploreASL folder and run

MichaelStritt · 2021-05-26T14:47:46Z

Test of the changes in `a6ded74` & `476f2c1`

>> source = '.\GE_PCASL_3Dspiral_volunteer';
>> [x] = ExploreASL_Master(source,1,[1 0 0]);
ExploreASL will run the import workflow and will run the processing pipeline...

...

====================================================================================
Many thanks for using ExploreASL, please don't forget to cite https://pubmed.ncbi.nlm.nih.gov/32526385/.
Note that ExploreASL is a collaborative effort.
Therefore, please don't hesitate to contribute by feedback, adding code snippets, or clinical experience!

>> x.logging
  1×2 struct array with fields:
    message
    stack

>> x.logging(1)
  struct with fields:
    message: 'I seem to be a bug in bids 2 legacy :)'
      stack: [3×1 struct]

>> x.logging(2)
  struct with fields:
    message: 'I seem to be a bug in the structural module :)'
      stack: [4×1 struct]

MichaelStritt · 2021-05-26T15:08:47Z

If you run ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/sourceStructure.json',[0 1 0 1],0) from the flavors, it should report that 'temp' is missing and shouldn't continue

Fixed in NII2BIDS f1186a0 👍

MichaelStritt · 2021-05-26T15:25:28Z

ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/studyPar.json',[1 0 0 0],0) reports no matching JSON found. Which is funny, because the JSON is there, it's a wrong JSON though - shouldn't it report something else?

I think this should be fixed with 54a3b6d. Right now the script always checks the JSON file name. If it's a sourceStructure, dataPar, studyPar or dataset_description file, it searchs for the studyRoot directory. So now you get a lot of warnings that you definitely shouldn't use this file but it kind of finds the studyRoot and runs the import anyway, if the sourceStructure.json is there...

source = '.\GE_PCASL_3Dspiral_Product_pharma\studyPar.json';
>> [x] = ExploreASL_Master(source,1,0);
...
Warning: You provided a descriptive JSON file. We recommend to use the study root folder instead... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 (line 542)
  In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 393)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
Warning: You provided the studyPar.json, which should never be the input... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 (line 553)
  In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 393)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
ExploreASL will run the import workflow and will run the initialization...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      DCM2NII NII2BIDS ANONYMIZE BIDS2LEGACY 
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
...

MichaelStritt · 2021-05-26T15:38:01Z

ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/sourceStructure.json',[0 0 0 1],0) - this works, but shouldn't it report that sourceStructure is incorrect and studyPar.json was used?

So normally we want to use folders all the time, right? So here it finds the sourceStructure.json, detects that it's a sourceStructure.json, gives a warning that you shouldn't use descriptive JSON files, then it finds the studyRoot and if the rawdata folder is there, it does Bids2Legacy anyway... Hm... Now the question is if you would want to do it Bids2Legacy anyway or not :D

Example with missing rawdata

>> source = '.\GE_PCASL_3Dspiral_Product_pharma\sourceStructure.json';
>> [x] = ExploreASL_Master(source,[0 0 0 1],0);
...
Warning: You provided a descriptive JSON file. We recommend to use the study root folder instead... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 (line 542)
  In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 393)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
ExploreASL will run the import workflow and will load the dataset...
No dataPar.json provided...

...

==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      BIDS2LEGACY 
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ... 
Warning: Import workflow is turned on, but at least one required JSON file is missing... 
> In ExploreASL_ImportMaster (line 87)
  In ExploreASL_Master (line 75) 
ExploreASL will run the initialization...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ...

Example with available rawdata

>> source = '.\GE_PCASL_3Dspiral_Product_pharma\sourceStructure.json';
>> [x] = ExploreASL_Master(source,[0 0 0 1],0);
Warning: You provided a descriptive JSON file. We recommend to use the study root folder instead... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 (line 542)
  In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 393)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
ExploreASL will run the import workflow and will load the dataset...
No dataPar.json provided...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      BIDS2LEGACY 
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ... 
There is no dataPar.json file in the study root directory. Default settings will be used...
Converting from BIDS to Legacy: .\GE_PCASL_3Dspiral_Product_phar100%
M0 parsed for .\GE_PCASL_3Dspiral_Product_pharma\derivatives\ExploreASL\sub-11\ASL_1\ASL4D.nii.gz
Creating default dataPar.json since file was not found...
Overwriting x.dir.dataPar...
ExploreASL will run the initialization...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ...

MichaelStritt · 2021-05-26T15:41:25Z

ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/studyPar.json',[0 0 0 1],0) - why this crashes? Incorrect JSON, but the other calls survive this...

The the changes in 54a3b6d this should work now.

Small example

>> source = '.\GE_PCASL_3Dspiral_Product_pharma\studyPar.json';
>> [x] = ExploreASL_Master(source,[0 0 0 1],0);
Warning: You provided a descriptive JSON file. We recommend to use the study root folder instead... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 (line 542)
  In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 393)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
Warning: You provided the studyPar.json, which should never be the input... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot_invalid_starting_2_0 (line 553)
  In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 393)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
ExploreASL will run the import workflow and will run the initialization...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      BIDS2LEGACY 
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ... 
There is no dataPar.json file in the study root directory. Default settings will be used...
Converting from BIDS to Legacy: .\GE_PCASL_3Dspiral_Product_phar100%
M0 parsed for .\GE_PCASL_3Dspiral_Product_pharma\derivatives\ExploreASL\sub-11\ASL_1\ASL4D.nii.gz
Creating default dataPar.json since file was not found...
Overwriting x.dir.dataPar...
ExploreASL will run the initialization...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...TLAB\m.stritt\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma
Import Modules      
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ...

MichaelStritt · 2021-05-26T15:53:48Z

ExploreASL('./BIDS/TmpConversion/GE_PCASL_3Dspiral_Product_pharma/derivatives',[0 0 0 0],1) This doesn't do anything - though if you remove 'derivatives' or add 'ExploreASL' it does run - perhaps not a good idea to look one directory up, but if it spots that it's in derivatives, it can check for ExploreASL folder and run

Please let us not increase the complexity even more :)

I added a separate warning with a hint on what to do 9675c9a 👍

Example

>> source = '.\GE_PCASL_3Dspiral_Product_pharma\derivatives';
>> [x] = ExploreASL_Master(source,0,1);
Neither the sourceStructure.json, dataset_description.json nor dataPar.json exist, ExploreASL will only be initialized...
Warning: Please do not provide the derivatives or ExploreASL folder. Use the study root directory instead... 
> In ExploreASL_Initialize>ExploreASL_Initialize_checkStudyRoot (line 453)
  In ExploreASL_Initialize (line 119)
  In ExploreASL_Master (line 70) 
ExploreASL will run the initialization...


==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
StudyRoot           ...t\Server_xASL\New_TestData\GE_PCASL_3Dspiral_Product_pharma\derivatives
Import Modules      
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ...

jan-petr

OK.

HenkMutsaerts

Also, would be good to rename /Development/ConfigFiles to /Development/ImportConfigFiles, which is clearer to the user. Always think of somebody who has not read your documentation yet, that is a good practice also for writing your paper ;)

HenkMutsaerts · 2021-05-27T04:24:59Z

.gitignore

 xASL_GUI_run.sh
-*Thumbs.db
+*Thumbs.db
+Testing/testConfig.json


Are you able to move this script outside ExploreASL at your side, before we know it you have a huge list for .gitignore ;) I would only put stuff here that are very general

This is meant for the integration testing. Instead of having our paths hardcoded in the full bids pipeline workflow, everyone can have their own testConfig.json file. The benefit with the gitignore is that everyone can have their own and can change it without committing etc. You can check out the description in the integration testing script. If you still disagree, let's talk about it in the abba meeting.

I have unresolved that. Not that I would disagree with Michael, but to give Henk the opportunity to resolve himself. Sorry - at least from my experience - if it is resolved, then I don't read the answer ;) Which would be a shame..

Sure, you can checkout the following script xASL_test_IntegrationTesting and if you check out the header, there is a description:

% ----------------------------------------------------------------------------------------------------------------------------------------------------- % DESCRIPTION: Integration testing script. Run the full pipeline on the flavor datasets using ExploreASL_Master. % % EXAMPLE: [TestResults, CheckResults] = xASL_test_IntegrationTesting; % % Your testConfig.json could look like this e.g.: % % { % "pathExploreASL": "...\\ExploreASL", % "pathTest": "...\\FlavorTests", % "cmdCloneFlavors": "git clone git@github.com:ExploreASL/FlavorDatabase.git" % } % % ----------------------------------------------------------------------------------------------------------------------------------------------------- % Copyright 2015-2021 ExploreASL

Nice, indeed, let's only resolve conversations if they don't need to be read. So once you have read this, you can resolve this issue ;)

Nice header, though I would add the path then for testConfig.json: like Your testConfig.json in /ExploreASL/Testing/ should ... Minor issue, \\ should be // right

Development/BIDS/BIDS_createTemplatesImportConfig.m

Development/ConfigFiles/ACTL.json

HenkMutsaerts

It looks like several changes were done to address this issue (and the issue of including ExploreASL_ImportMaster in ExploreASL_Master) while keeping previous functionality intact, but this becomes messy, some functionality will change, e.g. is bProcessData still relevant when we have [1 1 1] as 3rd input argument?

Also, It seems we should have 1) ExploreASL_Master 2) ExploreASL_Initialization 3) ExploreASL_ImportMaster 4) ExploreASL_ProcessingMaster. Now (1) contains (4) but not (3). So seems best to separate (4) as well in 1.8.0.

The nomenclature sourceStructure studyPar dataPar needs updating. First is about directory structure of the source folder, second about the BIDS parameters that are missing in the DICOM files, third about the options that we want to start ExploreASL_Master with. This is not clear from their names. Let's make this an issue for 1.8.0 as well.

Do we need xASL_imp_Anonymize as BIDS is already anonymous? Assuming that we will not have the temporary analysis folder anymore soon?

I cannot see the differences for the parts that were moved outside of ExploreASL_Initialize to xASL_init_*, did you edit them first in ExploreASL_Initialize in a separate commit before moving them out, so I can review that specific commit?

Development/ExploreASL_UpdateStructure.m

ExploreASL_ImportMaster.m

ExploreASL_Master.m

HenkMutsaerts · 2021-05-27T05:07:12Z

ExploreASL_Master.m

    % Print user feedback
-    if x.bProcessData==0 || x.bProcessData==2
-        if x.bProcessData==2 && nargout==0
+    if x.opts.bProcessData==0 || x.opts.bProcessData==2


This is now confusing, we have 4 import "modules" and 3 processing "modules" but we still have this boolean (instead of the [0 0 1] [1 0 1] etc) so we should probably homogenize this when we split a common starting script from ImportMaster and ProcessingMaster

No, please let me clarify this. For the import we have the [x x x x] array. If a single value is greater than 0, I set bImportData to 1. For the processing we have the [x x x] array. If a single value is greater than 0, I set bProcessData to 1. This is helpful for code readability, because otherwise we have a lot of statements with if sum([...])>0 etc. I don't like the possibility of bProcessData==2 either, but that's still a relic that we previously used if you only want to "load a dataset". It's exactly the behavior that you implemented a while ago, the only thing is that the variable name changed.

I endorse this ;)

True, it should then just become clear from the comments and header. Also, the bProcessData==2 is not necessary anymore, now this is the same as ExploreASL_Master('/path2folder', 0,0) right?

weird, now it works.... I answered in a comment below, copy from there: bProcessData is no ExploreASL_Master input parameter anymore, but we still use it to determine if we want to load the data or not. Maybe there is another nice way to do this, but it was like a nicely running system before, so we kept it.

It seems that that part would be

instead of

if x.bProcessData == 2

we use:

if exist(dataParjsonpath, 'var') && ~x.bProcessData

jan-petr · 2021-05-27T07:24:59Z

It looks like several changes were done to address this issue (and the issue of including ExploreASL_ImportMaster in ExploreASL_Master) while keeping previous functionality intact, but this becomes messy, some functionality will change, e.g. is bProcessData still relevant when we have [1 1 1] as 3rd input argument?

Also, It seems we should have 1) ExploreASL_Master 2) ExploreASL_Initialization 3) ExploreASL_ImportMaster 4) ExploreASL_ProcessingMaster. Now (1) contains (4) but not (3). So seems best to separate (4) as well in 1.8.0.

OK - not sure if I understand completely - can you create this issue for 1.8.0 @HenkMutsaerts ?

The nomenclature sourceStructure studyPar dataPar needs updating. First is about directory structure of the source folder, second about the BIDS parameters that are missing in the DICOM files, third about the options that we want to start ExploreASL_Master with. This is not clear from their names. Let's make this an issue for 1.8.0 as well.

OK. Lets make an issue - you talk about the file names themselves, or about how they are described? I would avoid changing the filenames too often - or we can simply rename them and issue a warning. Can you make that issue @HenkMutsaerts

Do we need xASL_imp_Anonymize as BIDS is already anonymous? Assuming that we will not have the temporary analysis folder anymore soon?

Sorry - wrong name - option [0 0 1 0] in import is not anonymize but deface. Do you mean this one, right? If yes, then let me know and I'll make an issue for that in 1.8.0.

I cannot see the differences for the parts that were moved outside of ExploreASL_Initialize to xASL_init_*, did you edit them first in ExploreASL_Initialize in a separate commit before moving them out, so I can review that specific commit?

Done and approved already in 1.6.0 - you need to reduce sleep to 2h per day to be able to review those as well :D

MichaelStritt · 2021-05-27T07:35:58Z

It looks like several changes were done to address this issue (and the issue of including ExploreASL_ImportMaster in ExploreASL_Master) while keeping previous functionality intact, but this becomes messy, some functionality will change, e.g. is bProcessData still relevant when we have [1 1 1] as 3rd input argument?

Also, It seems we should have 1) ExploreASL_Master 2) ExploreASL_Initialization 3) ExploreASL_ImportMaster 4) ExploreASL_ProcessingMaster. Now (1) contains (4) but not (3). So seems best to separate (4) as well in 1.8.0.

OK - not sure if I understand completely - can you create this issue for 1.8.0 @HenkMutsaerts ?

Me neither, so I agree with Jan. I also described the behavior of bProcessData and bImportData in one of the inline answers above. They are just helpful to quickly check if sum(ProcessModules)>0 or sum(ImportModules)>0, otherwise you would see this statement a hundred times in the initialize scripts.

The nomenclature sourceStructure studyPar dataPar needs updating. First is about directory structure of the source folder, second about the BIDS parameters that are missing in the DICOM files, third about the options that we want to start ExploreASL_Master with. This is not clear from their names. Let's make this an issue for 1.8.0 as well.

OK. Lets make an issue - you talk about the file names themselves, or about how they are described? I would avoid changing the filenames too often - or we can simply rename them and issue a warning. Can you make that issue @HenkMutsaerts

I really wouldn't change this. We call those in so many different places and the variable names are actually exactly like the file names, so pretty straightforward? I mean, they are basically the exact same names that are defined in BIDS, too, right? Why would you change this? I'd rather add nice descriptions to the documentation website, that's a lot more helpful than changing the names.

Do we need xASL_imp_Anonymize as BIDS is already anonymous? Assuming that we will not have the temporary analysis folder anymore soon?

Sorry - wrong name - option [0 0 1 0] in import is not anonymize but deface. Do you mean this one, right? If yes, then let me know and I'll make an issue for that in 1.8.0.

I think it's an actually cool feature that some people may want to use and other's maybe not. We previously discussed in two abba meetings that we want it as a part of the import workflow, I don't understand why we would suddenly change this???

I cannot see the differences for the parts that were moved outside of ExploreASL_Initialize to xASL_init_*, did you edit them first in ExploreASL_Initialize in a separate commit before moving them out, so I can review that specific commit?

Done and approved already in 1.6.0 - you need to reduce sleep to 2h per day to be able to review those as well :D

Agree 👍

MichaelStritt · 2021-05-27T08:46:11Z

@HenkMutsaerts & @jan-petr: I checked all the comments now. Everything should be answered and/or fixed. I did a minor restructure in 2e20982, because Henk was talking about the ImportMaster and that there is no ProcessMaster. Kind of makes sense to me. Other than that this should be it. If everyone is happy with names and comments now, I could do some additional testing.

jan-petr

OK.

HenkMutsaerts · 2021-05-27T12:51:10Z

.gitignore

 xASL_GUI_run.sh
-*Thumbs.db
+*Thumbs.db
+Testing/testConfig.json


Nice, indeed, let's only resolve conversations if they don't need to be read. So once you have read this, you can resolve this issue ;)

Nice header, though I would add the path then for testConfig.json: like Your testConfig.json in /ExploreASL/Testing/ should ... Minor issue, \\ should be // right

Development/ConfigFiles/ACTL.json

Development/ExploreASL_UpdateStructure.m

HenkMutsaerts · 2021-05-27T13:46:43Z

ExploreASL_Master.m

    % Print user feedback
-    if x.bProcessData==0 || x.bProcessData==2
-        if x.bProcessData==2 && nargout==0
+    if x.opts.bProcessData==0 || x.opts.bProcessData==2


True, it should then just become clear from the comments and header. Also, the bProcessData==2 is not necessary anymore, now this is the same as ExploreASL_Master('/path2folder', 0,0) right?

Modules/SubModule_Import/xASL_imp_DCM2NII_Subject_CopyTempDir.m

MichaelStritt · 2021-05-27T14:13:45Z

True, it should then just become clear from the comments and header. Also, the bProcessData==2 is not necessary anymore, now this is the same as ExploreASL_Master('/path2folder', 0,0) right?

Sorry, I couldn't comment under your review @HenkMutsaerts.
bProcessData is no ExploreASL_Master input parameter anymore, but we still use it to determine if we want to load the data or not. Maybe there is another nice way to do this, but it was like a nicely running system before, so we kept it.

MichaelStritt · 2021-05-27T14:26:57Z

I found it for you (please google yourself first next time ;) In general, it is good to study the BIDS documents, to understand what nomenclature they have. It doesn't stop with the JSON ;)

BIDS calls this the root "dataset" folder. So would be good if we call it dataset as well (so studyPar.json becomes datasetPar.json, and StudyRoot would then be datasetRoot

https://bids-specification.readthedocs.io/en/stable/02-common-principles.html#storage-of-derived-datasets

Oh, okay. I just thought we would do everything exactly like in the flavor database and there the files have these names.

…ging

…e code

MichaelStritt added import Related to data import module bids Moving ExploreASL to BIDS compatibility labels May 22, 2021

MichaelStritt requested review from HenkMutsaerts and jan-petr May 22, 2021 14:20

MichaelStritt self-assigned this May 22, 2021

MichaelStritt linked an issue May 22, 2021 that may be closed by this pull request

Release v1.7.0: Manage ExploreASL_Master input path #588

Closed

jan-petr requested changes May 25, 2021

View reviewed changes

MichaelStritt requested a review from jan-petr May 25, 2021 19:13

jan-petr requested changes May 25, 2021

View reviewed changes

MichaelStritt requested a review from jan-petr May 26, 2021 16:50

jan-petr approved these changes May 26, 2021

View reviewed changes

HenkMutsaerts changed the title ~~Feature #588 bids directory input~~ Feature #588 Manage ExploreASL input path May 27, 2021

HenkMutsaerts requested changes May 27, 2021

View reviewed changes

MichaelStritt requested a review from HenkMutsaerts May 27, 2021 08:49

jan-petr approved these changes May 27, 2021

View reviewed changes

HenkMutsaerts requested changes May 27, 2021

View reviewed changes

MichaelStritt and others added 5 commits May 27, 2021 21:37

#588 Rename analysis to temp, fix minor bugs related to new error log…

e5d851a

…ging

#588 Fix minor path related bugs

1bc373a

#588 Update ConfigFiles (AnalysisRoot -> TempRoot)

5a93347

#588 ExploreASL_Initialize: warn if missing ROOT dir

dff6e3e

#588 Use xASL_qc_AddLoggingInfo for both import & pipeline logging

d33f1ea

MichaelStritt force-pushed the feature-#588_BIDS_DirectoryInput branch from db5c327 to f295b6f Compare May 27, 2021 19:39

MichaelStritt added 15 commits May 27, 2021 21:39

#588 Minor fix for error logging

9407d20

#588 Fix (missing temp folder in NII2BIDS)

316b220

#588 Fix (studyPar.json as input parameter)

ed5aba9

#588 Fix (derivatives or ExploreASL directory)

19cf467

#588 Fix (analysis -> temp for unit test)

d8e6258

#588 Update description (ExploreASL_UpdateStructure)

afba931

#588 Update description (ExploreASL_ImportMaster)

b4abde3

#588 Update if statement (ExploreASL_ImportMaster)

85cd300

#588 Update variable name and description (ExploreASL_ImportMaster)

f9187a5

#588 Update comments (ExploreASL_Master)

63da310

#588 Update reinitialize statement (ExploreASL_Master)

2e33c3a

#588 Update file structure (ExploreASL_ProcessMaster)

c8f0e17

#588 Update version name in warnings for old import

173be0f

#588 Move scripts to root directory

c898403

#588 xASL_imp_DCM2NII_Subject_CopyTempDir: Add comments to clarify th…

04afbc7

…e code

MichaelStritt merged commit f295b6f into develop May 27, 2021

MichaelStritt added 7 commits May 27, 2021 21:39

#588 xASL_test_IntegrationTesting: Comment update

e8f6d94

#588 StudyRoot -> DatasetRoot

6b93dd4

#588 study -> dataset

8add65b

#588 x.opts.bOnlyLoad

08e33db

#588 Revert TempRoot -> AnalysisRoot

c818983

#588 ExploreASL_ImportConfig: AnalysisRoot -> TempRoot

526d0d6

#588 Fix problems related to bOnlyLoad

f295b6f

MichaelStritt deleted the feature-#588_BIDS_DirectoryInput branch May 27, 2021 19:39

Feature #588 Manage ExploreASL input path #596

Feature #588 Manage ExploreASL input path #596

Uh oh!

Conversation

MichaelStritt commented May 22, 2021

Linked issue

How to test

Comments

Uh oh!

MichaelStritt commented May 22, 2021

Major changes

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelStritt commented May 25, 2021

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelStritt commented May 26, 2021

Test of the changes in a6ded74 & 476f2c1

Uh oh!

MichaelStritt commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelStritt commented May 26, 2021

Uh oh!

MichaelStritt commented May 26, 2021

Example with missing rawdata

Example with available rawdata

Uh oh!

MichaelStritt commented May 26, 2021

Small example

Uh oh!

MichaelStritt commented May 26, 2021

Example

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

MichaelStritt May 27, 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!

Test of the changes in `a6ded74` & `476f2c1`

MichaelStritt commented May 26, 2021 •

edited

Loading

MichaelStritt May 27, 2021 •

edited

Loading