Fixes #834 Major flavor testing revamp #835

MichaelStritt · 2021-09-13T19:45:18Z

Linked issue

Read description in #834.

How to test

Check testing description in #834.

Comments

Let's improve this step by step.

MichaelStritt · 2021-09-14T11:19:22Z

Up-to-date exemplary output of bugs 🐛

>> flavors.loggingTable
ans =
  4×3 table

           message                             stack                                                name                                                      
    ______________________    _______________________________________    ____________________________________________________

    'this is a test error'    'xASL_module_Structural: line 42, ...'    '...\FlavorDatabase\GE_PCASL_3Dspiral_14.0LX_1'    
    'this is a test error'    'xASL_module_ASL: line 36, ...'           '...\FlavorDatabase\GE_PCASL_3Dspiral_14.0LX_1'    
    'this is a test error'    'xASL_module_Structural: line 42, ...'    '...\FlavorDatabase\GE_PCASL_3Dspiral_15.0LX_WIP_1'
    'this is a test error'    'xASL_module_ASL: line 36, ...'           '...\FlavorDatabase\GE_PCASL_3Dspiral_15.0LX_WIP_1'

jan-petr

I fixed two small bugs. All references are now correctly pushed. It all runs through without issues.

But:

When comparing Legacy with reference. Can we skip log files? Cause I didn't upload logs to reference. Missing: /ExploreASL/xASL_module_Import.log
At the end - the log returned no errors or warnings - which is strange or have we removed all the errors and warnings already?!

Other than that - ready to merge this one in the current state and do more changes elsewhere. Because we need to be able to work with new Flavors and Testing to fix other issues.

MichaelStritt · 2021-09-14T17:06:45Z

@jan-petr

When comparing Legacy with reference. Can we skip log files? Cause I didn't upload logs to reference. Missing: /ExploreASL/xASL_module_Import.log

Yeah, there are two solutions. Easiest is to create an optional input to ignore all .log files. The other method would be to return all differences in a struct and remove the log file differences there. Wasn't sure which one would be optimal. I definitely agree that this is necessary though. I removed the import logs as well when I was uploading the references.

At the end - the log returned no errors or warnings - which is strange or have we removed all the errors and warnings already?!

I think the BIDS comparison does not work 100% correctly. Don't know if we should work on this here though. I saw for one text file that they were not the same, it did report it, but the "identical" value was still "identical". I want to integrate this in a nicer way anyway, so maybe we do this later on. I just thought we need an initial new running script for all of this. Maybe we also delete xASL_test_IntegrationTesting while we're at it, because that's where some of those methods came from. There I was doing all of this a little bit differently, but I came up with this loggingTable stuff which can be quite useful. Right now we definitely don't return errors in the import, so at least the flavors.loggingTable wont return those buggy TopUp datasets and so on.

Other than that - ready to merge this one in the current state and do more changes elsewhere. Because we need to be able to work with new Flavors and Testing to fix other issues.

Agree, that's why this was the first thing I wanted to work on 👍

MichaelStritt · 2021-09-14T21:17:37Z

@jan-petr

Example with subset of flavors

Exemplary JSON file

{
   "pathExploreASL":     "...//ExploreASL",
   "pathFlavorDatabase": "...//FlavorDatabase",
   "cmdCloneFlavors":    "git clone git@github.com:ExploreASL/FlavorDatabase.git",
   "flavorList":         ["GE_PCASL_3Dspiral_14.0LX_1", "Philips_PCASL_2DEPI_3.2.1.1_1", "Siemens_PASL_3DGRASE_E11_1"]
}

Test errors in import and processing

flavors.loggingTable
ans =
  12×3 table

            message                                                                                                                                                  stack                                                                                                                                                       name              
    _______________________    _________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________    _______________________________

    'test error import'        'xASL_module_Import: line 226, ExploreASL_ImportMaster: line 36, ExploreASL_Master: line 71, ExploreASL: line 14, xASL_test_Flavors_DCM2BIDS: line 47, xASL_test_Flavors: line 74, xASL_test_FullPipelineTest: line 107'                                                             'GE_PCASL_3Dspiral_14.0LX_1'   
    'test error import'        'xASL_module_Import: line 226, ExploreASL_ImportMaster: line 36, ExploreASL_Master: line 71, ExploreASL: line 14, xASL_test_Flavors_DCM2BIDS: line 59, xASL_test_Flavors: line 74, xASL_test_FullPipelineTest: line 107'                                                             'GE_PCASL_3Dspiral_14.0LX_1'   
    'test error import'        'xASL_module_Import: line 226, ExploreASL_ImportMaster: line 36, ExploreASL_Master: line 71, ExploreASL: line 14, xASL_test_Flavors_DCM2BIDS: line 47, xASL_test_Flavors: line 74, xASL_test_FullPipelineTest: line 107'                                                             'Philips_PCASL_2DEPI_3.2.1.1_1'
    'test error import'        'xASL_module_Import: line 226, ExploreASL_ImportMaster: line 36, ExploreASL_Master: line 71, ExploreASL: line 14, xASL_test_Flavors_DCM2BIDS: line 59, xASL_test_Flavors: line 74, xASL_test_FullPipelineTest: line 107'                                                             'Philips_PCASL_2DEPI_3.2.1.1_1'
    'test error import'        'xASL_module_Import: line 226, ExploreASL_ImportMaster: line 36, ExploreASL_Master: line 71, ExploreASL: line 14, xASL_test_Flavors_DCM2BIDS: line 47, xASL_test_Flavors: line 74, xASL_test_FullPipelineTest: line 107'                                                             'Siemens_PASL_3DGRASE_E11_1'   
    'test error import'        'xASL_module_Import: line 226, ExploreASL_ImportMaster: line 36, ExploreASL_Master: line 71, ExploreASL: line 14, xASL_test_Flavors_DCM2BIDS: line 59, xASL_test_Flavors: line 74, xASL_test_FullPipelineTest: line 107'                                                             'Siemens_PASL_3DGRASE_E11_1'   
    'test error structural'    'xASL_module_Structural: line 42, runIteration: line 394, xASL_Iteration: line 100, ExploreASL_ProcessMaster: line 77, ExploreASL_Master: line 85, ExploreASL: line 14, xASL_test_Flavors_ExploreASL: line 13, xASL_test_Flavors: line 100, xASL_test_FullPipelineTest: line 119'    'GE_PCASL_3Dspiral_14.0LX_1'   
    'test error asl'           'xASL_module_ASL: line 36, runIteration: line 394, xASL_Iteration: line 100, ExploreASL_ProcessMaster: line 103, ExploreASL_Master: line 85, ExploreASL: line 14, xASL_test_Flavors_ExploreASL: line 13, xASL_test_Flavors: line 100, xASL_test_FullPipelineTest: line 119'          'GE_PCASL_3Dspiral_14.0LX_1'   
    'test error structural'    'xASL_module_Structural: line 42, runIteration: line 394, xASL_Iteration: line 100, ExploreASL_ProcessMaster: line 77, ExploreASL_Master: line 85, ExploreASL: line 14, xASL_test_Flavors_ExploreASL: line 13, xASL_test_Flavors: line 100, xASL_test_FullPipelineTest: line 119'    'Philips_PCASL_2DEPI_3.2.1.1_1'
    'test error asl'           'xASL_module_ASL: line 36, runIteration: line 394, xASL_Iteration: line 100, ExploreASL_ProcessMaster: line 103, ExploreASL_Master: line 85, ExploreASL: line 14, xASL_test_Flavors_ExploreASL: line 13, xASL_test_Flavors: line 100, xASL_test_FullPipelineTest: line 119'          'Philips_PCASL_2DEPI_3.2.1.1_1'
    'test error structural'    'xASL_module_Structural: line 42, runIteration: line 394, xASL_Iteration: line 100, ExploreASL_ProcessMaster: line 77, ExploreASL_Master: line 85, ExploreASL: line 14, xASL_test_Flavors_ExploreASL: line 13, xASL_test_Flavors: line 100, xASL_test_FullPipelineTest: line 119'    'Siemens_PASL_3DGRASE_E11_1'   
    'test error asl'           'xASL_module_ASL: line 36, runIteration: line 394, xASL_Iteration: line 100, ExploreASL_ProcessMaster: line 103, ExploreASL_Master: line 85, ExploreASL: line 14, xASL_test_Flavors_ExploreASL: line 13, xASL_test_Flavors: line 100, xASL_test_FullPipelineTest: line 119'          'Siemens_PASL_3DGRASE_E11_1'

jan-petr

Minor header stuff. I will do the testing now...

Functions/xASL_bids_CompareStoreDifference.m

Functions/xASL_bids_CompareStructures.m

Functions/xASL_bids_CompareStructuresCheckContent.m

Functions/xASL_bids_CompareStructuresJSON.m

Functions/xASL_bids_CompareStructuresNIFTI.m

Functions/xASL_bids_CompareStructuresTEXT.m

Functions/xASL_bids_CompareStructuresTSV.m

Functions/xASL_test_Flavors.m

Functions/xASL_test_Flavors_DCM2BIDS.m

jan-petr

Deleted comment :)

Functions/xASL_test_Flavors.m

HenkMutsaerts · 2021-09-16T08:41:53Z

@MichaelStritt the system call on line 82 in xASL_test_FullPipelineTest can be improved, e.g.:

[result1, result2] = system(...., '-echo');

result1 & result2 = 0 for correct system run and output in case of an error (can't remember the order :)
'echo' ensures that the user sees on the screen what system does, rather than waiting a long time without seeing feedback
compareStructures*: please write fully rather than using abbreviations, so I can also understand the code :) e.g., dn. Also variables like identical and differences are not self-explanatory?
Note that we also had in our wishlist to:
- Avoid copying sourcedata
- Allow unzipping sourcedata on scantype level (new request from Paul Groot for efficient storage)
- move some of the stuff that is in the test scripts here, to the actual ExploreASL import scripts. So the test script should run import scripts, not other test scripts
- Make the import more modular (Michael & I should plan a meeting on this, perhaps good to discuss this physically next week)
Note my minor bugfix commit in macOS test r1.8.0 import #842.

jan-petr

Some more tiny issues, but other than that - it's smoothly running...

Functions/xASL_test_Flavors.m

MichaelStritt · 2021-09-16T10:17:56Z

@HenkMutsaerts & @jan-petr

@MichaelStritt the system call on line 82 in xASL_test_FullPipelineTest can be improved, e.g.:

[result1, result2] = system(...., '-echo');

result1 & result2 = 0 for correct system run and output in case of an error (can't remember the order :)

'echo' ensures that the user sees on the screen what system does, rather than waiting a long time without seeing feedback

I personally never used that option. I have my local flavor database that works nicely as is. Feel free to change this, but IMHO this is not part of this issue.

compareStructures*: please write fully rather than using abbreviations, so I can also understand the code :) e.g., dn. Also variables like identical and differences are not self-explanatory?

There are detailed explanations now AND those variables have been there before. This issue is not about optimizing the naming. It's about getting the flavor testing running again.

Note that we also had in our wishlist to:

Avoid copying sourcedata

Yes, that's what the new implementation does.

Allow unzipping sourcedata on scantype level (new request from Paul Groot for efficient storage)

I made a comment about that in the corresponding issue. I'm against that and/or don't want to implement that.

move some of the stuff that is in the test scripts here, to the actual ExploreASL import scripts. So the test script should run import scripts, not other test scripts

This is done step by step. Jan & I are getting rid of as much of the specific configurations as possible. Next steps here are to add dataPar.jsons where we need them instead of doing this in the testing script. As far as I know there are only like two or three flavors left that really require manual interaction AND all of this is not part of this issue/PR.

Note my minor bugfix commit in macOS test r1.8.0 import #842.

Minor merge conflict incoming, but I think you'll be able to deal with this.

jan-petr

OK.

This change was on my to-do for a long time now. Especially while testing the different code bugs, we can see that this is required. The CreateFileReport script is not run within a try-catch statement. This is a main reason why it should never crash. Therefore we need proper checks here.

…arison

Originally I had 10 separate commits here, but it was getting a bit ugly. I squashed them together. I am mainly revamping xASL_bids_CompareStructures here and I created a unit test for it. There is also an update for the testing scripts related to the flavor testing and I moved a lot of functions to separate scripts.

We export nice tables now :)

MichaelStritt requested a review from jan-petr September 13, 2021 19:45

MichaelStritt self-assigned this Sep 13, 2021

MichaelStritt added the testing Either unit testing or full pipeline testing for bugs label Sep 13, 2021

MichaelStritt linked an issue Sep 13, 2021 that may be closed by this pull request

Flavor testing script for up-to-date database #834

Closed

jan-petr requested changes Sep 14, 2021

View reviewed changes

MichaelStritt requested a review from jan-petr September 14, 2021 21:23

MichaelStritt force-pushed the testing-#834_MasterScript branch from b595630 to c2607ae Compare September 15, 2021 11:15

MichaelStritt mentioned this pull request Sep 15, 2021

Lighter Flavor testing #822

Closed

jan-petr requested changes Sep 16, 2021

View reviewed changes

Functions/xASL_test_Flavors.m Show resolved Hide resolved

Functions/xASL_test_Flavors.m Show resolved Hide resolved

jan-petr requested changes Sep 16, 2021

View reviewed changes

Functions/xASL_test_Flavors.m Show resolved Hide resolved

Functions/xASL_test_Flavors.m Show resolved Hide resolved

MichaelStritt requested a review from jan-petr September 16, 2021 10:19

jan-petr approved these changes Sep 17, 2021

View reviewed changes

MichaelStritt and others added 11 commits September 17, 2021 13:50

#834 xASL_test_...: major revamp

0a3c693

#834 xASL_test_Flavors: update IO

bdf044e

#834 xASL_test_FullPipelineTest: update logging behavior

f279d7f

#834 Minor bug fix in Import

491cff1

#834 Move code to functions, use testConfig everywhere, simplify comp…

39360ab

…arison

#834 xASL_test_...: simplify code

bee330b

#834 xASL_bids_...: improve tsv and log behavior

8b843f6

#834 xASL_test_...: improve logging and overall testing

ab4e4d7

#834 Testing and comparison revamp 2

c3df295

We export nice tables now :)

MichaelStritt added 4 commits September 17, 2021 13:50

#834 xASL_test & xASL_bids: update headers and fix IO check

42fb388

#834 xASL_test & xASL_bids: update headers and fix IO check

6e18dcb

#834 xASL_test_Flavors: add fallback for flavorList

fdbefeb

#834 xASL_test_IgnoreFiles: ignore list of files

487052e

MichaelStritt force-pushed the testing-#834_MasterScript branch from fd2fa55 to 487052e Compare September 17, 2021 11:51

MichaelStritt merged commit 487052e into develop Sep 17, 2021

MichaelStritt deleted the testing-#834_MasterScript branch September 17, 2021 11:51

Fixes #834 Major flavor testing revamp #835

Fixes #834 Major flavor testing revamp #835

Uh oh!

Conversation

MichaelStritt commented Sep 13, 2021

Linked issue

How to test

Comments

Uh oh!

MichaelStritt commented Sep 14, 2021

Up-to-date exemplary output of bugs 🐛

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

MichaelStritt commented Sep 14, 2021

Example with subset of flavors

Exemplary JSON file

Test errors in import and processing

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!

jan-petr 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!

Uh oh!

Uh oh!

HenkMutsaerts commented Sep 16, 2021 • 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!

MichaelStritt commented Sep 16, 2021

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MichaelStritt commented Sep 14, 2021 •

edited

Loading

jan-petr left a comment •

edited

Loading

HenkMutsaerts commented Sep 16, 2021 •

edited

Loading