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

Move getSNRModalities() out of the profile file (prod file) #358

Merged
merged 4 commits into from
Jan 2, 2019

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Nov 28, 2018

Description

Moves the function getSNRModalities() out of the profile file. The list of modalities to compute SNR on will now be stored in the Config module of LORIS under the Imaging Pipeline Section as shown in the screenshot below:
screen shot 2018-11-28 at 3 50 12 pm

The function computeSNR() of MRIProcessingUtility.pm has been updated to grep that information from the Config module now instead of the profile file.

SQL for this changes are in aces/Loris#4159

This resolves...

Caveat for existing projects:

Make sure you have run the patch from this LORIS PR (aces/Loris#4159; link to be updated to final URL of the patch at the time of release documentation) and that you have updated the Config module with the modalities listed in the function getSNRModalities of your profile file.

From now on, the insertion scripts will rely on the values stored in the Config module instead of the profile file and the function getSNRModalities can be removed from your profile file.

To test this

  • Source the patch from the LORIS PR Added the modalities for which to compute SNR to the Config module Loris#4159
  • Run the tarchiveLoader and make sure it computes the SNR only for the modalities mentioned in the Config module
  • Run the tool BackPopulateSNRAndAcquisitionOrder.pl on one TarchiveID for which you have previously removed the SNR values from parameter_file and make sure it computes the SNR as expected

@cmadjar
Copy link
Collaborator Author

cmadjar commented Dec 3, 2018

I added the blocked tag for now until we agree on which milestone this PR and the LORIS one should go to. To follow up with @ridz1208

@cmadjar cmadjar closed this Dec 3, 2018
@cmadjar cmadjar reopened this Dec 3, 2018
@cmadjar cmadjar removed the Blocked Merge it and you die label Dec 3, 2018
@cmadjar
Copy link
Collaborator Author

cmadjar commented Dec 3, 2018

This should go to 20.2 so removing the Blocked label

my $base = basename($filename);
my $fullpath = "$data_dir/$filename";
my $message;
if ( grep(/$fileScanType/, @{$modalities}) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not check for equality instead of a pattern match, as in

if( grep($_ eq $fileScanType, @$modalities) ) {

If you have modalities like T1 and T1weighted and you are checking for the modalities that match pattern /T1/ then both will match and that's probably not what you want...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very good point! Changing it to equality.

Copy link
Collaborator Author

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@nicolasbrossard Thank you for the suggestion!! I integrated it and fixed the message inserted in the notification_spool table. Please re-review.

my $base = basename($filename);
my $fullpath = "$data_dir/$filename";
my $message;
if ( grep(/$fileScanType/, @{$modalities}) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very good point! Changing it to equality.

@cmadjar cmadjar merged commit 21d0f81 into aces:minor Jan 2, 2019
@cmadjar cmadjar deleted the move_getSNRmodalities_out_of_prod_file branch January 2, 2019 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants