Skip to content

Bug #713 structural module error #716

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
jan-petr merged 17 commits into from
Jul 9, 2021
Merged

Bug #713 structural module error #716

jan-petr merged 17 commits into from
Jul 9, 2021

Conversation

@jan-petr
Copy link
Contributor

@jan-petr jan-petr commented Jul 7, 2021

Linked issue

#713

@jan-petr jan-petr self-assigned this Jul 7, 2021
@jan-petr jan-petr linked an issue Jul 7, 2021 that may be closed by this pull request
Structural module: matrices multiplication is not working #713
Closed
@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jul 8, 2021 •
edited
Loading

I think I found my main bug. Within xASL_init_DataLoading I'm doing the loading of the dataPar.json, just like we normally did. So there we call xASL_init_LoadDataParameterFile. Within xASL_init_LoadDataParameterFile we read in x.D.ROOT.
In ExploreASL_Initialize before xASL_init_DataLoading, however, we run xASL_init_DefinePaths. This doesn't seem to be data independent at all, since it runs xASL_init_DefinePaths which itself requires x.D.ROOT.
The other bug is in xASL_init_checkDatasetRoot, which turns off data loading if there is no dataPar.json within derivatives yet. That one can be fixed a little bit easier I think.

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jul 8, 2021 •
edited
Loading

Philips_2DEPI_FLAIRsegmentation_PWI4D_QuantificationParameters_LoQ

image

These things can be fixed quite easily 👍

Fix 1: https://github.com/ExploreASL/TestDataSets/commit/2b0d792a2de119096287fee0b342879c45b5260c

Fix 2: https://github.com/ExploreASL/TestDataSets/commit/e4454d4f1481d8226f451991f378a1e3f8ff0c0e

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jul 8, 2021 •
edited
Loading

Bug that Jan discovered already

------------------------------------------------------------------------
CAT Preprocessing error for T1:
------------------------------------------------------------------------
Reference to non-existent field 'YlesionFull'.
------------------------------------------------------------------------
  611 - cat_main
  842 - cat_run_job
   21 - cat_run_newcatch
  866 - run_job
  451 - cat_run
   29 - cfg_run_cm
 1692 - local_runcj
  959 - cfg_util
  470 - fill_run_job
  248 - spm_jobman
  609 - xASL_wrp_CAT12Segmentation
  179 - xASL_wrp_SegmentT1w
  378 - xASL_module_Structural
  394 - runIteration
  100 - xASL_Iteration
   43 - ExploreASL_ProcessMaster
   82 - ExploreASL_Master
  340 - xASL_qc_TestExploreASL
------------------------------------------------------------------------

Print 'Graphics' figure to: 
  .\Philips_2DEPI_Lesion_NoFLAIR_NoM0_HiQ\Sub-0001\report\catreport_SPM12.pdf
  Remove ".\Philips_2DEPI_Lesion_NoFLAIR_NoM0_HiQ\Sub-0001\mri\p1T1.nii" 
from dependency list because it does not exist!
One or multiple files do not exist and were removed from the dependency list 
and following batches will may not work correctly!

  Remove ".\Philips_2DEPI_Lesion_NoFLAIR_NoM0_HiQ\Sub-0001\mri\p2T1.nii" 
from dependency list because it does not exist!
One or multiple files do not exist and were removed from the dependency list 
and following batches will may not work correctly!

  Remove ".\Philips_2DEPI_Lesion_NoFLAIR_NoM0_HiQ\Sub-0001\mri\p0T1.nii" 
from dependency list because it does not exist!
One or multiple files do not exist and were removed from the dependency list 
and following batches will may not work correctly!
...

Hopefully fixed in 9626dbb

@MichaelStritt MichaelStritt added the bug Something isn't working label Jul 8, 2021
BeatrizPadrela
BeatrizPadrela approved these changes Jul 9, 2021
View reviewed changes
Copy link
Contributor

@BeatrizPadrela BeatrizPadrela left a comment

Choose a reason for hiding this comment

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

Works on afivasc data!

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jul 9, 2021

Test datasets

image

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jul 9, 2021 •
edited
Loading

BIDS conversion test

image

Pipeline

Only one error and as far as I know that one was there before because the image was zeroed out?

Philips_PCASL_3DGRASE_functional
3D_GRASE sequence detected
ERROR: ASL module terminated for subject 1: sub-Patient1
ans =
Error using xASL_module_ASL line 220
Invalid ASL image

HenkMutsaerts
HenkMutsaerts approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@HenkMutsaerts HenkMutsaerts 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!

@HenkMutsaerts
Copy link
Member

HenkMutsaerts commented Jul 9, 2021

@MichaelStritt continues deserving more beer. Be careful not to work too hard or you'll be very drunk at the ExploreASL workshop in September ;)

@MichaelStritt MichaelStritt removed the request for review from MDijsselhof July 9, 2021 10:31
@jan-petr jan-petr merged commit 94a42c7 into develop Jul 9, 2021
@jan-petr jan-petr deleted the bug-#713_StructuralModuleError branch July 9, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HenkMutsaerts HenkMutsaerts HenkMutsaerts approved these changes

@BeatrizPadrela BeatrizPadrela BeatrizPadrela approved these changes

+1 more reviewer

@MichaelStritt MichaelStritt MichaelStritt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jan-petr jan-petr

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Structural module: matrices multiplication is not working

5 participants

@jan-petr @MichaelStritt @HenkMutsaerts @BeatrizPadrela