Fix logging of MRI violations log #327

cmadjar · 2018-07-23T20:18:16Z

This pull request fixes the logic of scan insertion into the mri_violations_log table that got compromised with the refactoring of mri_protocol_checks (no comma allowed anymore).

Description of the bug:

Let's say that for the MINC header xspace:length of a T1W, there are multiple validRange in the mri_protocol_checks like below:

For a T1W scan with a xspace:length of 176 (fitting the second row in the table above), the insertion pipeline will exclude the T1W scan from insertion and log an entry in the mri_violations_log table saying that the validRange for a T1W and this header is 10-20 (not taking into consideration the second valid range).

New logic of the pipeline after the fix (validated with Dave on Friday)

grep distinct Header values in the mri_protocol_checks table for the identified Scan_type
execute the following for both types of severity ("exclude" and "warning")

a) for each Header found in 1., grep all possible values (validRange and validRegex) that are permitted for this Scan_type and Severity and check whether the scan is valid based on these values
b) if the scan has been deemed not valid across all possible values for that Header, Scan_type and Severity, then log the proper information into the mri_violations_log table as only one entry. In the example below, there was two entries in the mri_protocol_checks for Header=zspace:length, Severity=warning and Scan_type=adniT1 (one for 10-20 and one for 30-40). It inserted the scan into mri_violations_log only once with a summary of the possible values for that Header, Severity and Scan_type.

behaviour depending on the extra validation status:

a) if the scan is classified as exclude, then the scan will not be inserted into the files table and logged into the mri_violations_log table and viewable through the MRI violations module.
b) if the scan is classified as warn, then the scan will be inserted into the files table with Caveat set to TRUE and logged into the mri_violations_log table (that can be seen through the MRI violations module)
c) if the scan is classified as pass, then the scan will be inserted into the files table without any Caveat set

…n CheckID from mri_violations_log since not used anywhere in the code)

MounaSafiHarab · 2018-07-25T20:23:24Z

@cmadjar
this has to be coordinated with the Loris PR as well (namely the mri violated scans module and how it displays the information)...
@llevitis talked to me earlier today.

I just want to make sure we are all on the same page with all these open PRs and what should stay and what should be closed;
I am confuseddddd!

nicolasbrossard · 2018-07-25T20:29:13Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

+
+        # grep all valid ranges and regex to compare with value in the file
+        my (@valid_ranges, @valid_regexs);
+        while (my $check = $sth->fetchrow_hashref()) {


I suggest removing the declaration of @valid_ranges and @valid_regexs and replacing with:

my @rows = @{ $sth->fetchall_arrayref( { ValidRange => 1, ValidRegex => 1 } ) }; my @valid_ranges = grep($_->{'ValidRange'}, @rows); my @valid_regexs = grep($_->{'ValidRegex'}, @rows);

Just remember that from now on both arrays contain references to the rows that contain non-null ranges/regexs.

nicolasbrossard · 2018-07-25T20:29:29Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

+        # type, severity and header
+        next if (!@valid_ranges && !@valid_regexs);
+
+        # loop through all checks


I would reduce this to

next if grep(NeuroDB::MRI::in_range($value, $_->{'ValidRange'}), @valid_ranges);

and if we get past this point, we know that @valid_ranges will only contain references to rows for which $value is not in range.

nicolasbrossard · 2018-07-25T20:29:39Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

+                push(@failed_valid_ranges, $valid_range);
+            }
+        }
+        foreach my $valid_regex (@valid_regexs) {


Same logic here:

next if grep($value =~ /$_->{'ValidRegex'}/, @valid_regexs);

and if we get past this point, we know that @valid_regexs will only contain references to rows for which $value does not match the regex for that row.

nicolasbrossard · 2018-07-25T20:35:43Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

+        # value in one of the valid ranges or regex set for that scan type,
+        # header and severity
+        next if $is_valid;
+


More concise IMHO

$valid_fields{$header} = { ScanType => $scan_type, HeaderValue => $value, ValidRanges => [ map { $_->{'ValidRange'} } @valid_ranges ], ValidRegexs => [ map { $_->{'ValidRegex'} } @valid_regexs ] };

nicolasbrossard

I am suggesting some code refactoring. Let me know what you think.

cmadjar · 2018-07-25T20:44:35Z

@cmadjarthis has to be coordinated with the Loris PR as well (namely the mri violated scans module and how it displays the information)...
@llevitis talked to me earlier today.

I just want to make sure we are all on the same page with all these open PRs and what should stay and what should be closed;
I am confuseddddd!

@MounaSafiHarab I know it gets confusing. This PR is only taking care of the comma normalization of mri_protocol_checks which have been pushed to 20.0 and is currently breaking on the LORIS-MRI front if projects had commas in their tables. The hyphen normalization will be pushed to minor and I will issue a PR to minor for this change when the time comes. Or I will push to your branch @MounaSafiHarab as I think you have a PR for the hyphen normalization of mri_protocol.

Does that clarify a bit?

cmadjar · 2018-07-26T13:11:46Z

@nicolasbrossard Thank you very much for the code suggestions!! Looks so much better now :).

Ready for another round of review when you are ready.

nicolasbrossard · 2018-07-26T15:03:32Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

 	  $this->{LOG}->print($message);
 	  # 'warn' and 'exclude' are errors, while 'pass' is not
 	  # log in the notification_spool_table the $Verbose flag accordingly
-	  if (!($checks[0] eq 'pass')){
+	  if (!($extra_validation_status eq 'pass')){


Nitpick:

if($extra_validation_status ne 'pass') {

nicolasbrossard

Some more refactoring that I didn't think about during the first pass.

nicolasbrossard · 2018-07-26T15:40:20Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm


-            }
+    # grep the excluded headers from mri_protocol_check for the scan type
+    my @exclude_headers;


I would replace the rest of the code for this function with this

foreach(qw/exclude warning/) { $sth->execute($scan_type, $severity); my @headers = map { $_->{'Header'} } @{ $sth->fetchall_arrayref({}) }; my %validFields = $this->loop_through_protocol_violations_checks( $scan_type, $severity, \@headers, $file ); if(%validFields) { $this->insert_into_mri_violations_log( \%validFields, $severity, $pname, $candID, $visit_label, $file ); return $severity; } } return 'pass';

cmadjar · 2018-07-26T16:03:48Z

@nicolasbrossard Thank you for the suggestions!! All done and ready for another review :)

MounaSafiHarab · 2018-08-01T20:15:48Z

@cmadjar
tested this and works perfectly!!!!
I tested with dummy data (I used the xpace:length you show in your example) and without your branch, it goes to violated scans logging only the valid range values in the first row of the table (whereas my scan matched the second row);
and with your changes, it goes to the files table (Imaging browser).

I will put passed manual tests but will wait for @nicolasbrossard to test it as well before merging

nicolasbrossard · 2018-08-02T20:27:52Z

I tested this PR "through" minc_insertion.pl and it works perfectly. I can see that all lines of mri_protocol_checks are taken into account during the extra checks and also that the exclude/warning messages display the concatenation of the valid ranges/regexes.

👍

nicolasbrossard

Little changes in the documentation

nicolasbrossard · 2018-08-02T16:46:03Z

docs/scripts_md/MRIProcessingUtility.md

-  - @checks               : array of extra checks
+  - $acquisitionProtocol     : acquisition protocol
+  - $acquisitionProtocolID   : acquisition protocol ID
+  - $extra\_validation\_status : extra validation status ("pass", "exclude", "warn")


Replace "warn" by "warning" (method was refactored)

nicolasbrossard · 2018-08-02T16:46:09Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

-  - @checks               : array of extra checks
+  - $acquisitionProtocol     : acquisition protocol
+  - $acquisitionProtocolID   : acquisition protocol ID
+  - $extra_validation_status : extra validation status ("pass", "exclude", "warn")


"warn" should be replaced by "warning"

cmadjar · 2018-08-02T20:33:48Z

@nicolasbrossard Thanks! Changes made :)

cmadjar added 5 commits July 18, 2018 18:36

Committing text.pl with changes to add in MRIProcessingUtility.pm

d3bb1e3

More progress on the algorithm (validated by Dave to delete the colum…

862aaf6

…n CheckID from mri_violations_log since not used anywhere in the code)

finished implementing changes to the logic for the extra checks

74429f9

deleting test.pl file from git

2922434

recreating the .md file for the changed documentation of this PR

429e5d0

cmadjar added Bug [branch] major labels Jul 23, 2018

cmadjar added this to the 20.0 (previously called 19.2) milestone Jul 23, 2018

cmadjar requested review from nicolasbrossard and MounaSafiHarab July 23, 2018 20:18

cmadjar changed the base branch from minor to 20.0-dev July 23, 2018 20:18

llevitis mentioned this pull request Jul 25, 2018

[Tools/SQL] Split mri_protocol range columns into min & max columns aces/Loris#3528

Closed

nicolasbrossard reviewed Jul 25, 2018

View reviewed changes

nicolasbrossard requested changes Jul 25, 2018

View reviewed changes

Nicolas's great suggestion for code simplification :)

6710d0b

nicolasbrossard reviewed Jul 26, 2018

View reviewed changes

nicolasbrossard requested changes Jul 26, 2018

View reviewed changes

additional great comments from Nicolas :)

07f6d05

leave warning be

9365d8e

nicolasbrossard approved these changes Jul 31, 2018

View reviewed changes

nicolasbrossard added the PassedCodeReview label Jul 31, 2018

MounaSafiHarab added the PassedManualTest label Aug 1, 2018

nicolasbrossard self-assigned this Aug 2, 2018

nicolasbrossard requested changes Aug 2, 2018

View reviewed changes

Nicolas' feedback

7c66f13

nicolasbrossard approved these changes Aug 2, 2018

View reviewed changes

nicolasbrossard merged commit 7235a5c into aces:20.0-dev Aug 2, 2018

cmadjar deleted the Fix_Logging_of_mri_violations_log branch August 2, 2018 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix logging of MRI violations log #327

Fix logging of MRI violations log #327

cmadjar commented Jul 23, 2018

MounaSafiHarab commented Jul 25, 2018

nicolasbrossard Jul 25, 2018

nicolasbrossard Jul 25, 2018

nicolasbrossard Jul 25, 2018

nicolasbrossard Jul 25, 2018 •

edited

Loading

nicolasbrossard left a comment

cmadjar commented Jul 25, 2018

cmadjar commented Jul 26, 2018

nicolasbrossard Jul 26, 2018

nicolasbrossard left a comment

nicolasbrossard Jul 26, 2018

cmadjar commented Jul 26, 2018

MounaSafiHarab commented Aug 1, 2018

nicolasbrossard commented Aug 2, 2018

nicolasbrossard left a comment

nicolasbrossard Aug 2, 2018

nicolasbrossard Aug 2, 2018

cmadjar commented Aug 2, 2018

Fix logging of MRI violations log #327

Fix logging of MRI violations log #327

Conversation

cmadjar commented Jul 23, 2018

Description of the bug:

New logic of the pipeline after the fix (validated with Dave on Friday)

MounaSafiHarab commented Jul 25, 2018

nicolasbrossard Jul 25, 2018

Choose a reason for hiding this comment

nicolasbrossard Jul 25, 2018

Choose a reason for hiding this comment

nicolasbrossard Jul 25, 2018

Choose a reason for hiding this comment

nicolasbrossard Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

nicolasbrossard left a comment

Choose a reason for hiding this comment

cmadjar commented Jul 25, 2018

cmadjar commented Jul 26, 2018

nicolasbrossard Jul 26, 2018

Choose a reason for hiding this comment

nicolasbrossard left a comment

Choose a reason for hiding this comment

nicolasbrossard Jul 26, 2018

Choose a reason for hiding this comment

cmadjar commented Jul 26, 2018

MounaSafiHarab commented Aug 1, 2018

nicolasbrossard commented Aug 2, 2018

nicolasbrossard left a comment

Choose a reason for hiding this comment

nicolasbrossard Aug 2, 2018

Choose a reason for hiding this comment

nicolasbrossard Aug 2, 2018

Choose a reason for hiding this comment

cmadjar commented Aug 2, 2018

nicolasbrossard Jul 25, 2018 •

edited

Loading