-
Notifications
You must be signed in to change notification settings - Fork 50
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
Creation of a generic file insertion script #271
Conversation
* Mapping empty date of birth and scan to undef when empty so that it can be inserted in MySQL 5.7 (aces#205) * Creected code that tests if two float numbers are equal (aces#206) * [Cleanup]: Remove duplicate DBI.pm; Redmine 13613 (aces#207) * Add TarchiveID to mri_protocol_violated_scans (aces#211) * Adding TarchiveID information in mri_protocol_violated_scans table * Cleaned up commented parts * Renamed $tarchiveInfo to $tarchiveInfoRef for consistencies * Get file list after removal of undesired directories like __MACOSX: Redmine 13508 (aces#203) * Get file list after removal of undesired directories like __MACOSX: Redmine 13508 * change xargs to deleting from the find command * Dave and Nick feedback * Nicolas feedback; escaping the directory name * Refactor smart matching code in DTI.pm (aces#215) * Better MINC toolkit sourcing command in README and updated VERSION file (aces#260) * Better MINC toolkit sourcing command and updated VERSION file * cecile feedback * Updating QC tables if the -deleteQCdata flag is not set. Will update FileID to NULL in files_qcstatus and feedback_mri_comments (aces#261) * README has also within it LORIS-MRI version number, so update (aces#268)
…nd exit. More input checking and parsing metadata are required as well
…ript Conflicts: uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm
documented the script using Perldoc added support to read a JSON file to insert metadata into parameter_file. Also fixed a bug in MRIProcessingUtility.pm introduced by a conflict when merging latest version of aces/minor renamed file_insertion.pl to imaging_non_minc_insertion.pl modified the notifications to use imaging non minc file insertion forgot to rename the .md file to imaging_non_minc_insertion.md also missed renaming the script in mass_perldoc script updated documentation and help section of the script with the correct name of the script
4b625d6
to
f7b3e5f
Compare
…#4144) This accompanies the LORIS-MRI changes to insert non MINC files into the database (aces/Loris-MRI#271). In order for the notification for this new script to save in the notification table, we have to add one row to the notification_types table.
There was a problem hiding this 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 great feedback! I integrated all your comments. Ready for another review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PapillonMcGill Thank you for the comments. They should all be taken care of in the last commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1169,9 +1169,12 @@ sub registerScanIntoDB { | |||
######################################################## | |||
### update mri_acquisition_dates table ################# | |||
######################################################## | |||
my $acquisition_date = $tarchiveInfo->{'DateAcquired'} | |||
// $${minc_file}->getParameter('AcquisitionDate') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is not directly related to your code but I think there is no point in having method registerScanIntoDB
(the method that contains the code above) receive a reference to a MINC file. Since $minc_file
is a File
object, it already is a reference. I think every time you see a double dollar sign access (as in $${minc_file}
), it is a sign that too many levels of indirection are used...Anyway, leave it like this for now and let's try to "keep it cheap" in future PRs and minimize the number of $
used when accessing variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree!
I think that for this PR to work, you need the new module |
There was a problem hiding this 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 good catches!! Fixed now. Ready for another review.
All the issues have been addressed. Tagging as PassedCodeReview. 👍 |
This is a first iteration of a generic file insertion script. The script will insert files that are not MINC files into the files and parameter_file tables. Will probably still be modifying it as I will implement the PET HRRT insertion script and EEG insertion script which is why I tagged it as "Needs Work Before Merging".
Refers to following Redmine ticket: https://redmine.cbrain.mcgill.ca/issues/10557
PR sent to the LORIS repo to add
imaging non minc file insertion
to thenotification_types
table: aces/Loris#4144Notes:
Notes for existing projects
In order to be able to use this new script, you need to install the CPAN JSON library: