Skip to content

Fixes #850 xASL_wrp_RealingASL: field checks #851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
MichaelStritt merged 8 commits into from
Oct 3, 2021
Merged

Fixes #850 xASL_wrp_RealingASL: field checks #851

MichaelStritt merged 8 commits into from
Oct 3, 2021

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

4 participants

@MichaelStritt @BeatrizPadrela @jan-petr