GUI bug fixes and new GUI buttons #358

tanguyduval · 2019-09-24T14:45:18Z

Purpose

[BUG] Solve #357
[NEW] bvec and bval files can be loaded for Diffusion protocols (need DiffusionData in Model.MRIinputs). Now any diffusion dataset with .bvec and .bval files can be loaded easily in qMRLab
[NEW] Create button for protocol (e.g. linspace)
[NEW] Default button in the option panel can reset the protocol

Approach

Concerning #357 I just set NaN values to 0

Open Questions and Pre-Merge TODOs

Use github checklists. When solved, check the box and explain the answer.
Review that changed source files/lines are related to the pull request/issue
If any files/commits were accidentally included, cherry-pick them into another branch.
Review that changed source files/lines were not accidentally deleted
Fix appropriately if so.
Test new features or bug fix
If not implemented/resolved adequately, solve it or inform the developer by requesting changes in your review.
Preferably, set breakpoints in the locations that the code was changed and follow allong line by line to see if the code behaves as intended.

Manual GUI tests (general)

Specifications

Version:
Platform:
Subsystem:

coveralls · 2019-09-24T16:17:13Z

Coverage decreased (-0.1%) to 42.911% when pulling 7316f4d on cosmetic into cd30473 on master.

mathieuboudreau · 2019-09-25T12:28:24Z

Thanks @tanguyduval ! As for the NaN in masks, I'm worried about automatically setting values to 0; it removes some UI feedback telling us that there might be something wrong in the dataset (if there are NaNs in masks, I suspect that there is...). Also, QSM is very sensitive to the quality of the brain mask, and the algorithm could break under some circumstances if datapoints in the masks are not connected.

Maybe an alternative could be a popup appearing while initially loading the masked data, notifying the user about the issues in the mask data ("FYI: there are some points in the mask that are NaN. What do you want us to do with them ? (1) ignore (2) set to 0 (3) set to 1). What do you think?

agahkarakuzu · 2019-09-25T12:31:12Z

("FYI: there are some points in the mask that are NaN. What do you want us to do with them ? (1) ignore (2) set to 0 (3) set to 1). What do you think?

NaN values are dangerous in masks or in images that are being masked! Logical operations go schrodinger's cat and yield unexpected results, at least in Matlab I have nasty experiences :)

I think those values can be set to zero and a xxx_nan_mask binary image can be exported to inform the user about which values were changed from NaN --> 0.

agahkarakuzu · 2019-09-25T12:48:44Z

src/Common/FitData.m

@@ -77,6 +77,7 @@

    % Find voxels that are not empty
    if isfield(data,'Mask') && (~isempty(data.Mask))
+        data.Mask(isnan(data.Mask))=0;


This seems quite reasonable to me. Would saving isnan(data.Mask) output be extra?

mathieuboudreau · 2019-09-25T12:57:42Z

("FYI: there are some points in the mask that are NaN. What do you want us to do with them ? (1) ignore (2) set to 0 (3) set to 1). What do you think?

NaN values are dangerous in masks or in images that are being masked! Logical operations go schrodinger's cat and yield unexpected results, at least in Matlab I have nasty experiences :)

I think those values can be set to zero and a xxx_nan_mask binary image can be exported to inform the user about which values were changed from NaN --> 0.

I agree that they are "dangerous", but I'm afraid that automatically handling this might then create some issues long term if we don't inform the user that there was NaN in the masks. The dangerous thing was that they made a mistake in creating the mask in the first place (NaN should not have been there at all, so what else went wrong before) ! Automatically setting to 0 could cause some issues, like in QSM, where I could see some problems with the deconvolution if there are a lot of sparse 0 in the brain because some NaN appeared there. QSM is extremely sensitive to boundary conditions, and so I could envision a situation where someone complains that their QSM processed maps are aweful, but everything looks fine, and it's because we automatically set the NaNs in the brain to 0. I will be very hard to debug/discover, if we dont at a bare minimum inform the user that NaNs in the data exists, and what we did about it. For example, using a popup.

mathieuboudreau

Add functionality that informs user that NaNs in the data exists, and how we tried to resolve it.

agahkarakuzu · 2019-09-25T13:00:12Z

@mathieuboudreau I'd say we avoid pop-up, carry on with processing by NaN --> 0 but report/log it.

tanguyduval · 2019-09-25T13:22:31Z

@mathieuboudreau @agahkarakuzu I propose to use the input wait of FitData. If present, a warning dialog will open and recommend to check mask,. Otherwise, a warning will be displayed in the command Window.

agahkarakuzu · 2019-09-25T16:20:08Z

@tanguyduval for non-voxelwise models this is not taking effect. I tired it with vfa_t1.

Should we include this in SanityCheck ?

tanguyduval · 2019-09-25T17:27:20Z

@agahkarakuzu good catch. Solved in a3c1eb4

agahkarakuzu · 2019-09-25T21:24:03Z

I pushed a small commit to show warning only when NaN exists and also add this check to the voxelwise as well.

Checked functionality with vfa_t1 and ir_t1.

@mathieuboudreau quick question, can you fit mt_sat via GUI ?

tanguyduval · 2019-09-25T21:30:11Z

I pushed a small commit to show warning only when NaN exists

Oups... my mistake

and also add this check to the voxelwise as well.

I think you have duplicated the check for voxelwise... the first check is already before the condition if voxelwise

Checked functionality with vfa_t1 and ir_t1.

@mathieuboudreau quick question, can you fit mt_sat via GUI ?

in b1_dam

mathieuboudreau · 2019-09-30T17:50:52Z

Looks like PR #355 caused a lot of issues, there was the NaN issue that a user reported in #357, but @TommyBoshkovski let me know today that a lot of our models were also broken due to a similar issue (but not displaying the same error). I also discovered a completely different bug that happened for b1_dam, which I fixed in 7316f4d. This makes me suspect that there might be some more lingering around, so please be vigilant and report any GUI-related oddities if you encounter them; I had tested the GUI while reviewing #355, but only for one model (VFA), which worked fine. I think it may be overkill to test all models at every PR, so I won't update the checklist, but if ever any of you review a PR that specifically deals with the GUI backend (like imtools), then I think it may be prudent to do a more thorough review.

mathieuboudreau · 2019-09-30T17:52:52Z

@tanguyduval is this branch ready to be merged, or do you have more local changes to push?

tanguyduval · 2019-09-30T17:57:37Z

@mathieuboudreau this branch can be merged (and should be merged ASAP). I solved the problem of NaN at periphery here: 83d1b69
Don't know why the tests did not catch this, I'll investigate
Thanks for 7316f4d, I'll integrate it to imtool3D_td

mathieuboudreau

Approved after further testing & bug fixes.

mathieuboudreau · 2019-09-30T18:01:08Z

@tanguyduval I saw that commit about the NaNs at periphery, but don't fully understand what the issue was by reading the thread here. Can you expand on it?

I think Travis didn't catch these errors as most appear to deal with some GUI & data relationships, so maybe the CLI don't call the same tools during the tests.

mathieuboudreau · 2019-09-30T18:01:23Z

Thanks for all the help @tanguyduval !

tanguyduval · 2019-09-30T18:09:20Z

@mathieuboudreau nii_xform, in addition to loading files, applies an affine transform to match the reference. The interpolation that applies during the transformation introduce NaNs at periphery.

In qMRLab, we load each file independently (the reference was the same file that the one that was loaded). I modified the loading function to just load and skip the affine transform in this case.

And you are right, the CLI don't call this part...

mathieuboudreau · 2019-09-30T18:10:30Z

Great, thanks for clarifying!

tanguyduval added 11 commits September 19, 2019 13:19

Accept bvec/bvals

7fde1eb

use fprintf instead of disp

ed26d31

dti if no repeated b0

1605803

Create Protocol button

bb12adf

Manage NaN in Mask

91641cf

Manage abscence of header (.mat files)

b03a515

save Model options and protocol when switching Method

7122943

Ask if user want to reset Protocol

b1f52fd

intempestive message if no multiple b0 scans

1edccb3

save header after fit

2460379

error if no header in Current Data

9ecd84a

agahkarakuzu approved these changes Sep 25, 2019

View reviewed changes

mathieuboudreau self-requested a review September 25, 2019 12:57

mathieuboudreau requested changes Sep 25, 2019

View reviewed changes

tanguyduval added 2 commits September 25, 2019 15:32

Add warning

9706ac3

[BUG] Create button not working on T1 IR

fc18463

Move NaN in Mask at the top

a3c1eb4

Strengthen condition and add check to voxelwise as well

b90e0bc

tanguyduval added 2 commits September 27, 2019 19:20

Prevent NaN at periphery

83d1b69

Error in fields in Statistics

6fc4542

tanguyduval and others added 2 commits September 30, 2019 15:32

Fields are reversed...

188acef

[BUG] Fix error Undefined function or variable 'useForm'.

7316f4d

in b1_dam

mathieuboudreau self-requested a review September 30, 2019 17:58

mathieuboudreau approved these changes Sep 30, 2019

View reviewed changes

mathieuboudreau changed the title ~~Cosmetic~~ GUI bug fixes and new GUI buttons Sep 30, 2019

mathieuboudreau merged commit 2b2b9d8 into master Sep 30, 2019

mathieuboudreau deleted the cosmetic branch September 30, 2019 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUI bug fixes and new GUI buttons #358

GUI bug fixes and new GUI buttons #358

tanguyduval commented Sep 24, 2019 •

edited by agahkarakuzu

coveralls commented Sep 24, 2019 •

edited

mathieuboudreau commented Sep 25, 2019

agahkarakuzu commented Sep 25, 2019 •

edited

agahkarakuzu Sep 25, 2019

mathieuboudreau commented Sep 25, 2019

mathieuboudreau left a comment

agahkarakuzu commented Sep 25, 2019

tanguyduval commented Sep 25, 2019

agahkarakuzu commented Sep 25, 2019

tanguyduval commented Sep 25, 2019

agahkarakuzu commented Sep 25, 2019 •

edited

tanguyduval commented Sep 25, 2019

mathieuboudreau commented Sep 30, 2019

mathieuboudreau commented Sep 30, 2019

tanguyduval commented Sep 30, 2019 •

edited

mathieuboudreau left a comment

mathieuboudreau commented Sep 30, 2019

mathieuboudreau commented Sep 30, 2019

tanguyduval commented Sep 30, 2019

mathieuboudreau commented Sep 30, 2019

GUI bug fixes and new GUI buttons #358

GUI bug fixes and new GUI buttons #358

Conversation

tanguyduval commented Sep 24, 2019 • edited by agahkarakuzu

Purpose

Approach

Open Questions and Pre-Merge TODOs

Manual GUI tests (general)

Specifications

coveralls commented Sep 24, 2019 • edited

mathieuboudreau commented Sep 25, 2019

agahkarakuzu commented Sep 25, 2019 • edited

agahkarakuzu Sep 25, 2019

Choose a reason for hiding this comment

mathieuboudreau commented Sep 25, 2019

mathieuboudreau left a comment

Choose a reason for hiding this comment

agahkarakuzu commented Sep 25, 2019

tanguyduval commented Sep 25, 2019

agahkarakuzu commented Sep 25, 2019

tanguyduval commented Sep 25, 2019

agahkarakuzu commented Sep 25, 2019 • edited

tanguyduval commented Sep 25, 2019

mathieuboudreau commented Sep 30, 2019

mathieuboudreau commented Sep 30, 2019

tanguyduval commented Sep 30, 2019 • edited

mathieuboudreau left a comment

Choose a reason for hiding this comment

mathieuboudreau commented Sep 30, 2019

mathieuboudreau commented Sep 30, 2019

tanguyduval commented Sep 30, 2019

mathieuboudreau commented Sep 30, 2019

tanguyduval commented Sep 24, 2019 •

edited by agahkarakuzu

coveralls commented Sep 24, 2019 •

edited

agahkarakuzu commented Sep 25, 2019 •

edited

agahkarakuzu commented Sep 25, 2019 •

edited

tanguyduval commented Sep 30, 2019 •

edited