Skip to content

Fixes #850 xASL_wrp_RealingASL: field checks#851

Merged
MichaelStritt merged 8 commits intodevelopfrom
bug-#850_CheckField
Oct 3, 2021
Merged

Fixes #850 xASL_wrp_RealingASL: field checks#851
MichaelStritt merged 8 commits intodevelopfrom
bug-#850_CheckField

Conversation

@MichaelStritt
Copy link
Contributor

@MichaelStritt MichaelStritt commented Sep 24, 2021 •
edited
Loading

Linked issue

Check out #850

How to test

Develop without this fix should basically crash for almost every non-Hadamard dataset. Just do some basic testing of the ASL module using the test dataset. Alternatively you can always run the 10 test datasets, but that's probably overkill for such a small change.

Comments

@BeatrizPadrela: Okay, same here as with #849, please check if everything works correctly. If yes, then approve & move it to @jan-petr's bucket.

@MichaelStritt MichaelStritt added the bug Something isn't working label Sep 24, 2021
@MichaelStritt MichaelStritt linked an issue Sep 24, 2021 that may be closed by this pull request
Hadamard or TimeEncoded field check required #850
Closed
@MichaelStritt MichaelStritt changed the base branch from main to develop September 24, 2021 11:26
@MichaelStritt MichaelStritt changed the title Fixes #850 Field checks Fixes #850 xASL_wrp_RealingASL: field checks Sep 24, 2021
@MichaelStritt
Copy link
Contributor Author

MichaelStritt commented Sep 27, 2021

Unit testing

There still seems to be a small bug in there.

====================================== 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_CompareStructures'                   'function'    [1x1  struct]    false 
    '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]    false 
    'xASL_module_ASL'                               'module'      [1x1  struct]    true  
    'xASL_module_Population'                        'module'      [1x1  struct]    true  
    'xASL_module_Structural'                        'module'      [1x1  struct]    true  

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

@MichaelStritt
Copy link
Contributor Author

MichaelStritt commented Sep 28, 2021

@jan-petr & @BeatrizPadrela: all bugs fixed, unit tests pass now 👍 weird that you didn't see those bugs during testing of the multi-TE feature

BeatrizPadrela
BeatrizPadrela approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@BeatrizPadrela BeatrizPadrela left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced HadamardType for TimeEncodedMatrixType, which is the variable name we use. (#858 branch was created to correct this, but maybe we can delete it because it's corrected here now)

@BeatrizPadrela BeatrizPadrela mentioned this pull request Sep 30, 2021
Closed
jan-petr
jan-petr requested changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - but 1 thing can be optimized and one is not entirely correct.

@BeatrizPadrela
Copy link
Contributor

BeatrizPadrela commented Sep 30, 2021 •
edited
Loading

I processed one twin from baseline, and the structural + ASL modules are fine. I was having issues with ASL module, but it's all fine now

image

image

I can also run UnitTesting if you prefer

@BeatrizPadrela BeatrizPadrela self-requested a review September 30, 2021 15:00
@BeatrizPadrela
Copy link
Contributor

BeatrizPadrela commented Oct 1, 2021

Unit Testing:

image

jan-petr
jan-petr requested changes Oct 1, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small request with the numel - otherwise looks good.

@MichaelStritt MichaelStritt requested a review from jan-petr October 2, 2021 20:55
jan-petr
jan-petr approved these changes Oct 2, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good now. Though I hope I didn't make it unstable with the last optimization tips ;)

@MichaelStritt
Copy link
Contributor Author

MichaelStritt commented Oct 3, 2021

Looks very good now. Though I hope I didn't make it unstable with the last optimization tips ;)

Don't worry 😆 The unit test has four different scenarios and all of them worked. In addition it's only the comparison script, which isn't used in the processing pipeline at all 👍

@MichaelStritt MichaelStritt merged commit d75d16a into develop Oct 3, 2021
@MichaelStritt MichaelStritt deleted the bug-#850_CheckField branch October 3, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jan-petr jan-petr jan-petr approved these changes

@BeatrizPadrela BeatrizPadrela BeatrizPadrela approved these changes

Assignees

@BeatrizPadrela BeatrizPadrela

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Hadamard or TimeEncoded field check required

3 participants

@MichaelStritt @BeatrizPadrela @jan-petr