Skip to content
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.

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

Error catching #169

Merged
merged 56 commits into from
Mar 7, 2018
Merged

Error catching #169

merged 56 commits into from
Mar 7, 2018

Conversation

ileppe
Copy link
Member

@ileppe ileppe commented Feb 23, 2018

  • Some general error-catching functionality that pops up a window to let the user know the input was not provided correctly.
  • Please see issue Error checking #156 for cases currently handled and try to break it! ;)

Ilana Leppert and others added 30 commits February 8, 2018 16:20
Also, correct qMRusage for voxelwise=0 methods.
@tanguyduval
Copy link
Member

tanguyduval commented Feb 28, 2018

load_nii handles single slice perfectly. Issue can happen if the nifti (or .mat data) were badly saved. We should add this to the doc and explain how to repair the nifti in this case.
Maybe also clarify the error in the SanityCheck if we suspect this error (e.g. if the first 2 dimensions match but not the 3rd one).

@tanguyduval
Copy link
Member

Travis is fixed, @ileppe I'm done with this branch. I let you clarify the SanityCheck warnings for singleton dimensions if you want (see previous message). Thanks!

@mathieuboudreau
Copy link
Member

mathieuboudreau commented Feb 28, 2018

@tanguyduval Even if load_nii currently works as expected, we should also anticipate what happens if it doesn't, like it did to @agahkarakuzu .

It should be simple to ensure that the number of dimensions matches the qMRLab expected number of dimensions right when the data is loaded, by comparing the size of the loaded data and adding a singleton dimension if missing (e.g. if size of data is 3 ([x,y,t]), add a singleton dimension ([x,y,1,t])). That way if in the future we expand i/o compatibility to other formats like MAT files/MINC/DICOM, or if a future version of load_nii breaks single slice cases for NIFTI files, we'll be prepared for it.

But we can continue that discussion on the other issue page, let's not bloat this current branch with too many unrelated changes like we did for new_doc.

@ileppe
Copy link
Member Author

ileppe commented Feb 28, 2018

@tanguyduval, why did you change to break instead of return out of SanityCheck? Right now it does not exit once it finds an error, it goes on to all checks. For e.g. I would like the following code to stop execution and throw an error (i.e.missing a necessary input).

Model = vfa_t1; 
FlipAngle = [3.0000; 20.0000];
TR = [0.0150; 0.0150];
Model.Prot.VFAData.Mat = [ FlipAngle TR];
data = struct();
data.B1map=double(load_nii_data('qMRLab/Data/vfa_t1_demo/vfa_t1_data/B1map.nii.gz'));
FitResults = FitData(data,Model,0);

Instead, it goes on and crashes in the next error check. I hope that's clear.

@tanguyduval
Copy link
Member

tanguyduval commented Feb 28, 2018

@mathieuboudreau if load_nii does not load correctly a single slice, we have to solve this at the source. If we handle bad input data, that might be a bit of a headache in some cases. For instance, we cannot differentiate between these two inputs: single 3D volume, and multiple 2D slices stack in the 3rd dimension. Just a feeling, I'm ok with any solution

@ileppe If an error appears, it DOES NOT go on to all checks, but breaks and throw an errordlg if no output to SanityCheck.
You are right, that does not stop the code... but that's necessary until I find a solution to the get_optional_inputson Octave (currently it believes that everything is mandatory)...

@mathieuboudreau
Copy link
Member

@tanguyduval I agree, that's fine with me then. However, we should still investigate deeper into why @agahkarakuzu encountered this in the first place. We were lucky he's a dev that encountered this problem, if he was another user, he might not have reached out to us. We need to try to ensure that whatever lead to him having this issue is not repeated by other people after the ISMRM release, whether that be changes in the code or changes in the documentation on how to load/save data.

@agahkarakuzu let's continue this discussion on the other issue. Could you give a more detailed explanation as to how we could reproduce this error you encountered regarding singleton dimensions?

@ileppe
Copy link
Member Author

ileppe commented Feb 28, 2018

@tanguyduval ok, sorry I was not clear, but the problem is that it breaks out of the for loop but does not throw an errordlg and continues to crash later on. i.e. Picks up an error here (Line 72):
https://github.com/neuropoly/qMRLab/blob/30e7603464a39e795aa8e5c14e7d16d1a3603956/Common/AbstractModel.m#L67-L76

Does not throw an errordlg and crashes here (line 79):

https://github.com/neuropoly/qMRLab/blob/30e7603464a39e795aa8e5c14e7d16d1a3603956/Common/AbstractModel.m#L79

Giving the user no info about the missing input problem.

@tanguyduval
Copy link
Member

@ileppe argh... I see the issue, it's because of the nested loops. It does not break the parent loop, only the first loop for ii=1:length(optionalInputs) . Return was nice but we had to create the errordlg afterward which was not perfect neither. I solved this in 3a8f821

agahkarakuzu
agahkarakuzu previously approved these changes Mar 1, 2018
Ilana Leppert added 2 commits March 1, 2018 11:08
@ileppe
Copy link
Member Author

ileppe commented Mar 7, 2018

@tanguyduval please check and approve this one too pls!

@ileppe ileppe requested a review from agahkarakuzu March 7, 2018 16:18
Copy link
Member

@agahkarakuzu agahkarakuzu left a comment

Choose a reason for hiding this comment

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

Hooray! We are sane.

Regarding dimension issue discussed in this PR, please see this suggestion to confirm if it is also sane :)

agahkarakuzu
agahkarakuzu previously approved these changes Mar 7, 2018
@ileppe ileppe merged commit 6b3589e into master Mar 7, 2018
@mathieuboudreau mathieuboudreau deleted the error-catch branch March 7, 2018 22:49
@tanguyduval
Copy link
Member

@ileppe looked good to me too, good job! Thanks Agah for reviewing it :)

@agahkarakuzu agahkarakuzu mentioned this pull request Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants