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.

Sign up for GitHub

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

Jump to bottom

Imaging Preprocessing Scripts: Added a ScannerID column to files table: Redmine 10939 #2189

Merged
merged 10 commits into from
Sep 28, 2016
Merged

Imaging Preprocessing Scripts: Added a ScannerID column to files table: Redmine 10939 #2189

merged 10 commits into from
Sep 28, 2016

Conversation

MounaSafiHarab
Copy link
Contributor

@MounaSafiHarab MounaSafiHarab commented Sep 14, 2016 •
edited
Loading

To be used with Loris-MRI pull request: aces/Loris-MRI#156

@MounaSafiHarab MounaSafiHarab added Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects Language: SQL PR or issue that update SQL code State: Needs work PR awaiting additional work by the author to proceed labels Sep 14, 2016
@MounaSafiHarab MounaSafiHarab added this to the 17.0 milestone Sep 14, 2016
@MounaSafiHarab MounaSafiHarab mentioned this pull request Sep 14, 2016
Merged
@codecov-io
Copy link

codecov-io commented Sep 14, 2016 •
edited
Loading

Current coverage is 13.76% (diff: 0.00%)

Merging #2189 into 17.0-dev will increase coverage by 0.04%

@@           17.0-dev      #2189   diff @@
==========================================
  Files           118        121     +3   
  Lines         20107      20242   +135   
  Methods        1124       1132     +8   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2759       2787    +28   
- Misses        17348      17455   +107   
  Partials          0          0

Powered by Codecov. Last update b6cbaaf...5ced65d

@driusan
Copy link
Collaborator

driusan commented Sep 14, 2016

Shouldn't we just call the field something more like 'ScannerID' and link it directly to the primary key ID column instead of getting more dependent on the CandID hack for phantoms?

driusan
driusan requested changes Sep 14, 2016
View reviewed changes
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

Could use a minor change, but I'm mostly setting this to "Request changes" to see how it works.

SQL/0000-00-00-schema.sql
@@ -448,6 +448,7 @@ CREATE TABLE `files` (
`ProcessProtocolID` int(11) unsigned,
`Caveat` tinyint(1) default NULL,
`TarchiveSource` int(11) default NULL,
`ScannerCandID` int(11) DEFAULT NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be a foreign key to the ID column, and not the CandID column of the mri_scanner table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

SQL/0000-00-00-schema.sql
@@ -458,7 +459,8 @@ CREATE TABLE `files` (
CONSTRAINT `FK_files_2` FOREIGN KEY (`AcquisitionProtocolID`) REFERENCES `mri_scan_type` (`ID`),
CONSTRAINT `FK_files_1` FOREIGN KEY (`SessionID`) REFERENCES `session` (`ID`),
CONSTRAINT `FK_files_3` FOREIGN KEY (`SourceFileID`) REFERENCES `files` (`FileID`),
CONSTRAINT `FK_files_4` FOREIGN KEY (`ProcessProtocolID`) REFERENCES `mri_processing_protocol` (`ProcessProtocolID`)
CONSTRAINT `FK_files_4` FOREIGN KEY (`ProcessProtocolID`) REFERENCES `mri_processing_protocol` (`ProcessProtocolID`),
CONSTRAINT `files_scannerID_fk` FOREIGN KEY (`ScannerCandID`) REFERENCES `mri_scanner` (`CandID`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a second comment to test GitHub's new code review feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MounaSafiHarab
Copy link
Contributor Author

@driusan

@driusan
Copy link
Collaborator

driusan commented Sep 15, 2016

I didn't mean the scanner ID (the serial number of the scanner from the dicom header) I meant the column named mri_scanner.ID (the primary key for the table.. which already exists.)

I think the convention to answer is to reply to the comment saying "Done". I don't know if GitHub has any specific built-in way to do it.

@MounaSafiHarab
Copy link
Contributor Author

@driusan,

yes this is exactly what I did, the ID of the mri_scanner...
this is already in parameter_file table.

@MounaSafiHarab MounaSafiHarab removed the State: Needs work PR awaiting additional work by the author to proceed label Sep 15, 2016
@MounaSafiHarab MounaSafiHarab changed the title Imaging Preprocessing Scripts: Added a ScannerCandID column to files table: Redmine 10939 Imaging Preprocessing Scripts: Added a ScannerID column to files table: Redmine 10939 Sep 16, 2016
@MounaSafiHarab
Copy link
Contributor Author

@gluneau
While testing, please check the CouchDB_MRI_Importer.php; I did not test this

@driusan driusan merged commit 0103d8b into aces:17.0-dev Sep 28, 2016
MounaSafiHarab
MounaSafiHarab commented Sep 30, 2016
View reviewed changes
SQL/Archive/17.0/2016-09-13-AddScannerIDInFiles.sql
@@ -0,0 +1,5 @@
-- Add ScannerCandID column to files, back-populate the newly added column from parameter_file table, then add foreign key constraints
ALTER TABLE files ADD `ScannerID` int(10) unsigned NOT NULL default '0';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set this to NULL (default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in: #2251

@MounaSafiHarab MounaSafiHarab mentioned this pull request Oct 4, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan approved these changes

Assignees

@gluneau gluneau

Labels
Language: SQL PR or issue that update SQL code Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MounaSafiHarab @codecov-io @driusan @gluneau