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

allow the insertion of violated scans #141

Merged
merged 7 commits into from Aug 25, 2016

Conversation

gluneau
Copy link
Contributor

@gluneau gluneau commented Jul 5, 2016

This command will insert a violated scan since providing the acquisition protocol bypasses the protocol checks. If you also need to bypass the extra_file_checks use bypass_extra_file_checks
Please note the -create_minc_pics option as well since it is usually created by the tarchiveloader script.

minc_insertion.pl -acquisition_protocol t2w -bypass_extra_file_checks -create_minc_pics -profile prod -globLocation -force  -tarchivePath /Users/xxxproject/dataTransfer/library/2009/DCM_2009-09-25_project_20110214_185904581.tar -mincPath /data/project/data/trashbin/TarLoad-3-34-pVzGC5/xxx0067_703739_v12_20090925_222403_18e1_mri.mnc

############# Create minc-pics #############################
############################################################
unless ($create_minc_pics)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

@gluneau,

you want to create pics if the user spoecifies this option; this will do the opposite
(I just confirmed when running the command line: pics generated withOUT specifying -create_minc_pics)

@MounaSafiHarab
Copy link
Contributor

@gluneau,

when running the command we should not specify -create_minc_pics 1; it should simply be -create_minc_pics (without the 1)

my ($acquisitionProtocol,$acquisitionProtocolID,@checks)
= $utility->getAcquisitionProtocol(
if(!defined($acquisitionProtocol)) {
my ($acquisitionProtocol,$acquisitionProtocolID,@checks)
Copy link
Contributor

Choose a reason for hiding this comment

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

@gluneau,

$acquisitionProtocol is no longer accessible for the next check $utility->registerScanIntoDB() (line 416)


["-create_minc_pics", "boolean", 1, \$create_minc_pics,
"Use this if invoked directly witouth tarchiveloader."],
Copy link
Collaborator

Choose a reason for hiding this comment

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

The help should describe what the flag does, not when to use it. ("without" is also spelled wrong.)

@MounaSafiHarab
Copy link
Contributor

@gluneau;

I think:

  1. you forgot to add the MRI.pm file to your last commit (which has the main create_minc_pics());
    and
  2. tarchiveLoader should be updated as well by removing the call to mass_pic.pl

@gluneau
Copy link
Contributor Author

gluneau commented Aug 1, 2016

@MounaSafiHarab

  1. The MRI.pm call was already present in the file see other make_ function calls. (line 16)

  2. The tarchieveLoader modification is in this request: Move minc pics creation to MRI.pm #149

@MounaSafiHarab
Copy link
Contributor

MounaSafiHarab commented Aug 9, 2016

@gluneau

This has to be tested together with the changes in: #149
otherwise "Undefined subroutine &NeuroDB::MRI::make_minc_pics called at uploadNeuroDB/minc_insertion.pl line 473."

Otherwise, it looks good.

(a minor comment: I would modify the comment as to how to run it by removing the leading 'perl' before the rest of the command, as this is how tarchiveLoader script runs minc_insertion.pl)

@gluneau
Copy link
Contributor Author

gluneau commented Aug 10, 2016

@MounaSafiHarab running instruction updated to not have the perl program in front.

@driusan
Copy link
Collaborator

driusan commented Aug 11, 2016

Acquisition_protocol as a parameter should only specify the acqusition_protocol, not also bypass the protocol enforcement.

There should be two flags:

  1. -acquisition_protocol (to specify the acquisition protocol if the mri_protocol table can't determine it)
  2. -force (to skip the checks and insert it regardless of the study protocol)

@MounaSafiHarab
Copy link
Contributor

@driusan @gluneau

the -force flag is already used when checking if the isTarchiveValidated flag is set in the mri_upload table, so I suggested to Greg to use another flag that is indicative of forcing specifically no extra_file_check. I assume you are Okay with this @driusan ?

@driusan
Copy link
Collaborator

driusan commented Aug 16, 2016

Do the flags get passed between the scripts? If not, I'm not sure it would be a problem and -force is a pretty standard name across different software packages. But if there's a conflict or you think it would be confusing, I have no problem using a different name (-forceinsert, maybe?)

@MounaSafiHarab
Copy link
Contributor

MounaSafiHarab commented Aug 16, 2016

Currently our imaging_upload_file.pl which runs all the insertion scripts calls tarchiveLoader withOUT the -force option. As we have it now, the -force is used to bypass tarchive validation checks in tarchiveLoader and bypass the isTarchiveValidated flag check in mri_upload table in the minc_insertion script.

I think we have two options:

  1. If it is to be used as is (i.e. -force), then a) the help section which describes the use of this flag needs to be updated to say that we are bypassing tarchiveValidation and extra_file_checks(), and b) projects that might have customized this flag for their study need to be made aware of this
  2. Use a flag name that is more specific to what we are doing: namely bypassing extra_file_checks() (as suggested by @gluneau) or -forceinsert (as syuggested by @driusan)

I personally prefer option (2)! it is more flags, but it is clearer I think!

@MounaSafiHarab
Copy link
Contributor

@gluneau

tested it on a file that would violate the mri_protocol_checks entry, with and without the -bypass_extra_file_checks flag, and it seems to be NOT inserting without the flag and inserting with the flag on. So I think this is all good now.

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.

None yet

4 participants