Fixes #827 xASL_module_Population: avoid running parallelized by HenkMutsaerts · Pull Request #838 · ExploreASL/ExploreASL

HenkMutsaerts · 2021-09-14T15:56:43Z

Linked issue

Closes #827

MichaelStritt

Seems good to me 👍
Did you test it?

Edit: during the last release we agreed on describing how to test and/or to show a small test that you ran, so that we do not have to fix that many bugs during the release

Edit2: please also fill in the release notes

MichaelStritt · 2021-09-24T10:00:58Z

@HenkMutsaerts: I did some minor changes:

Simplifications: Writing fprintf('%s\n', text) is kind of unnecessary if there is no variable, we can write fprintf('text\n') instead (ed968c4, 973eb76)
Legacy code: For the initialization and import I've tried to put more and more legacy code that will be removed with 2.0.0 to separate subfunctions, so that we can get rid of that more easily in the future. I did the same here, but we should still test this (b8e6514).
Existence check: I added 'file' here, to help other developers who read the code (c65d45c).

Do you know a nice way to test all of this?

HenkMutsaerts · 2021-09-25T10:23:29Z

Nice!

Moving legacy code to a separate function: I don't know from the top of my head if this is all legacy code that can be skipped, or if it should be replaced with something. And it needs to be carefully tested indeed, but I don't how all the use cases, there are too many :) So unless you know exactly how to test this in all examples, I would revert this subfunction.
I like it that we can have "minor" and "super minor" -> reminder for when you write papers: when you write a paper, you don't want to use superlatives when not needed.

MichaelStritt · 2021-09-27T07:12:29Z

Moving legacy code to a separate function: I don't know from the top of my head if this is all legacy code that can be skipped, or if it should be replaced with something. And it needs to be carefully tested indeed, but I don't how all the use cases, there are too many :) So unless you know exactly how to test this in all examples, I would revert this subfunction.

I mean, you wrote the comment that the following part is legacy code that should be removed later on. I think it would be the wrong signal if we just add a small subfunction and we say we can't do this because we don't know how to test this. I checked the IO parameters, they seem to be fine. I can run the unit testing and the test datasets at the AMC. Just thought that you might have something simpler in mind.

MichaelStritt · 2021-09-27T08:40:50Z

Unit testing

====================================== TEST RESULTS ======================================

                        name                           unit           tests        passed
    ____________________________________________    __________    _____________    ______

    'xASL_adm_CatchNumbersFromString'               'function'    [1x3  struct]    true  
    'xASL_adm_CheckFileCount'                       'function'    [1x2  struct]    true  
    'xASL_adm_CompareLists'                         'function'    [1x2  struct]    true  
    'xASL_adm_CorrectName'                          'function'    [1x3  struct]    true  
    'xASL_adm_DeleteFileList'                       'function'    [1x2  struct]    true  
    'xASL_adm_OrderFields'                          'function'    [1x1  struct]    true  
    'xASL_bids_BIDS2Legacy'                         'function'    [1x2  struct]    true  
    'xASL_bids_CheckDatasetDescription'             'function'    [1x2  struct]    true  
    'xASL_bids_Config'                              'function'    [1x1  struct]    true  
    'xASL_bids_CreateDatasetDescriptionTemplate'    'function'    [1x2  struct]    true  
    'xASL_bids_JsonCheck'                           'function'    [1x3  struct]    true  
    'xASL_bids_VendorFieldCheck'                    'function'    [1x1  struct]    true  
    'xASL_io_ExportVTK'                             'function'    [1x5  struct]    true  
    'xASL_io_Nifti2Im'                              'function'    [1x1  struct]    true  
    'xASL_stat_PSNR'                                'function'    [1x2  struct]    true  
    'xASL_stat_QuantileNan'                         'function'    [1x1  struct]    true  
    'xASL_stat_StdNan'                              'function'    [1x3  struct]    true  
    'xASL_str2num'                                  'function'    [1x3  struct]    true  
    'xASL_test_GetLogContent'                       'function'    [1x2  struct]    true  
    'xASL_tsvRead'                                  'function'    [1x2  struct]    true  
    'xASL_tsvWrite'                                 'function'    [1x2  struct]    true  
    'xASL_ExploreASL_Master'                        'master'      [1x11 struct]    true  
    'xASL_module_ASL'                               'module'      [1x1  struct]    true  
    'xASL_module_Population'                        'module'      [1x1  struct]    true  
    'xASL_module_Structural'                        'module'      [1x1  struct]    true  

==========================================================================================

MichaelStritt · 2021-09-27T13:12:04Z

10 test datasets

>> ResultsTable

ResultsTable = 

  Columns 1 through 8

    'Data'                   'mean_qCBF_TotalGM'    'median_qCBF_TotalGM'    'median_qCBF_DeepWM'    'CoV_qCBF_TotalGM'    'GMvol'     'WMvol'     'CSFvol'
    'GE_3Dspiral_T1pre…'    '64.51'                '52.2732'                '32.0559'               '0.29926'             [0.6400]    [0.5054]    [0.4330]
    'Philips_2DEPI_FEA…'    '38.9729'              '27.6587'                '6.73'                  '0.71513'             [0.5349]    [0.4590]    [0.4056]
    'Philips_2DEPI_FLA…'    '74.0946'              '56.3948'                '22.9226'               '0.33662'             [0.5442]    [0.3920]    [0.3561]
    'Philips_2DEPI_FLA…'    '48.959'               '43.4651'                '20.3422'               '0.41853'             [0.6994]    [0.5816]    [0.2222]
    'Philips_2DEPI_Les…'    'empty'                '14.4365'                '3.4282'                '0.89241'             [0.7096]    [0.5136]    [0.1942]
    'Philips_2DEPI_Seg…'    '45.9512'              '41.1554'                '9.4098'                '0.36523'             [0.8167]    [0.4596]    [0.1516]
    'Philips_3DGRASE_F…'    '35.1324'              '26.2474'                '7.6225'                '0.66049'             [0.5716]    [0.5282]    [0.4207]
    'Philips_3DGRASE_M…'    '31.9461'              '24.3224'                '8.5892'                '0.43733'             [0.7239]    [0.5520]    [0.3564]
    'Siemens3DGRASE_St…'    '37.6957'              '37.2872'                '21.7901'               '0.78873'             [0.5191]    [0.4658]    [0.4054]
    'Siemens_2DEPI_noB…'    '67.5016'              '50.6498'                '13.4243'               '0.31698'             [0.6886]    [0.6030]    [0.3101]

  Columns 9 through 11

    'PipelineCompleted'    'TC_ASL_Registration'    'TC_M0_Registration'
    [                1]    [             0.8995]    [            0.9315]
    [                1]    [             0.7292]    [            0.8137]
    [                1]    [             0.9194]    [            0.9479]
    [                1]    [             0.8141]    [            0.8344]
    [                1]    [             0.7443]    [            0.8897]
    [                1]    [             0.8544]    [            0.8028]
    [                1]    [             0.8568]    [            0.7545]
    [                1]    [             0.8389]    [            0.8932]
    [                1]    [             0.6915]    [            0.4987]
    [                1]    [             0.8757]    [            0.9589]

>> ResultsComparison.xASL_v1_7_0

ans = 

  Columns 1 through 8

    'Data'                   'mean_qCBF_TotalGM'    'median_qCBF_TotalGM'    'median_qCBF_DeepWM'    'CoV_qCBF_TotalGM'    'GMvol'    'WMvol'    'CSFvol'
    'GE_3Dspiral_T1pre…'    [                1]    [                  1]    [                 1]    [               1]    [    1]    [    1]    [     1]
    'Philips_2DEPI_FEA…'    [                1]    [                  1]    [                 1]    [               1]    [    1]    [    1]    [     1]
    'Philips_2DEPI_FLA…'    [                0]    [                  0]    [                 0]    [               0]    [    1]    [    1]    [     1]
    'Philips_2DEPI_FLA…'    [                1]    [                  1]    [                 0]    [               0]    [    1]    [    1]    [     1]
    'Philips_2DEPI_Les…'    [                1]    [                  0]    [                 0]    [               1]    [    1]    [    1]    [     1]
    'Philips_2DEPI_Seg…'    [                1]    [                  1]    [                 1]    [               1]    [    1]    [    1]    [     1]
    'Philips_3DGRASE_F…'    [                1]    [                  1]    [                 1]    [               0]    [    1]    [    1]    [     1]
    'Philips_3DGRASE_M…'    [                1]    [                  1]    [                 1]    [               0]    [    1]    [    1]    [     1]
    'Siemens3DGRASE_St…'    [                1]    [                  1]    [                 1]    [               1]    [    1]    [    1]    [     1]
    'Siemens_2DEPI_noB…'    [                1]    [                  1]    [                 1]    [               1]    [    1]    [    1]    [     1]

  Columns 9 through 11

    'PipelineCompleted'    'TC_ASL_Registration'    'TC_M0_Registration'
    [                1]    [                  1]    [                 1]
    [                1]    [                  1]    [                 1]
    [                1]    [                  0]    [                 1]
    [                1]    [                  0]    [                 0]
    [                1]    [                  0]    [                 1]
    [                1]    [                  1]    [                 1]
    [                1]    [                  1]    [                 1]
    [                1]    [                  1]    [                 1]
    [                1]    [                  1]    [                 1]
    [                1]    [                  1]    [                 1]

MichaelStritt

As can be seen in the unit testing and the 10 test datasets, everything should be fine. The deviations from the correct values in the 10 test datasets are because this branch does not have the up-to-date reference table.

…bility mask Which is equal to the brainmask

…of x.Q.Sequence

…esponse

HenkMutsaerts requested a review from MichaelStritt September 14, 2021 15:56

HenkMutsaerts assigned MichaelStritt Sep 14, 2021

HenkMutsaerts linked an issue Sep 14, 2021 that may be closed by this pull request

Population module becomes x.Q.Sequence independent #827

Closed

MichaelStritt approved these changes Sep 14, 2021

View reviewed changes

MichaelStritt added the bug Something isn't working label Sep 14, 2021

MichaelStritt self-requested a review September 15, 2021 12:31

MichaelStritt changed the title ~~#827 xASL_module_Population: avoid running parallelized~~ Fixes #827 xASL_module_Population: avoid running parallelized Sep 24, 2021

MichaelStritt approved these changes Sep 27, 2021

View reviewed changes

HenkMutsaerts and others added 11 commits September 27, 2021 15:19

#827 xASL_module_Population: avoid running parallelized

673aa06

#827 xASL_wrp_CreateAnalysisMask: for 3D spiral create dummy suscepti…

7643b84

…bility mask Which is equal to the brainmask

#827 xASL_wrp_Quantify: Always use susceptibility mask, irrespective …

0e9df3f

…of x.Q.Sequence

#827 xASL_module_Population: Remove x.Q.Sequence dependency

028e8ad

#827 xASL_stat_GetROIstatistics: remove x.Q.Sequence dependency

86f1aea

#827 xASL_im_CreateAnalysisMask: remove x.Q.Sequence dependency

2e01c9e

#827 xASL_module_Population: minor cosmetics for print statements

af95f03

#827 xASL_im_CreateAnalysisMask: use subfunction for legacy code

90360e6

#827 xASL_stat_GetROIStatistics: super minor cosmetics

3ebb0e5

#827 xASL_wrp_Quantify: super minor cosmetics

87db8f1

#827 xASL_im_CreateAnalysisMask: minor cosmetic changes after Henks r…

5a888b9

…esponse

MichaelStritt force-pushed the bug-#827_Population_Parallel branch from c57d407 to 5a888b9 Compare September 27, 2021 13:20

MichaelStritt merged commit 5a888b9 into develop Sep 27, 2021

MichaelStritt deleted the bug-#827_Population_Parallel branch September 27, 2021 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #827 xASL_module_Population: avoid running parallelized#838

Fixes #827 xASL_module_Population: avoid running parallelized#838
MichaelStritt merged 11 commits intodevelopfrom
bug-#827_Population_Parallel

HenkMutsaerts commented Sep 14, 2021

Uh oh!

MichaelStritt left a comment •

edited

Loading

Uh oh!

MichaelStritt commented Sep 24, 2021

Uh oh!

HenkMutsaerts commented Sep 25, 2021 •

edited by MichaelStritt

Loading

Uh oh!

MichaelStritt commented Sep 27, 2021

Uh oh!

MichaelStritt commented Sep 27, 2021

Uh oh!

MichaelStritt commented Sep 27, 2021

Uh oh!

MichaelStritt left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

HenkMutsaerts commented Sep 14, 2021

Linked issue

Uh oh!

MichaelStritt left a comment • 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!

MichaelStritt commented Sep 24, 2021

Uh oh!

HenkMutsaerts commented Sep 25, 2021 • edited by MichaelStritt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelStritt commented Sep 27, 2021

Uh oh!

MichaelStritt commented Sep 27, 2021

Unit testing

Uh oh!

MichaelStritt commented Sep 27, 2021

10 test datasets

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MichaelStritt left a comment •

edited

Loading

HenkMutsaerts commented Sep 25, 2021 •

edited by MichaelStritt

Loading