-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add support for multiple MRI protocols. #4321
Add support for multiple MRI protocols. #4321
Conversation
* | ||
* @return array list of the last MRI protocol IDs for each group. | ||
*/ | ||
function getLastProtocolIDInGroup($mriProtocolData) |
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.
function getLastProtocolIDInGroup($mriProtocolData) | |
function getLastProtocolIDInGroup($mriProtocolData): array |
@nicolasbrossard |
@kongtiaowang Thanks Shen. I tried searching for that error message in the build log and I could not find it...How did you find the error? |
SQL/0000-00-00-schema.sql
Outdated
@@ -724,6 +732,38 @@ INSERT INTO mri_protocol (Center_name,Scan_type,TR_range,TE_range,time_range) VA | |||
('ZZZZ',44,'2000-2500','2-5',NULL), | |||
('ZZZZ',45,'3000-9000','100-550',NULL); | |||
|
|||
CREATE TABLE `mri_protocol_group` ( |
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.
It's difficult to review this PR because these tables are new and meaning of them/their intended purposes isn't really defined anywhere.. can you maybe put a comment on each explaining what they're for and what they mean, for lack of a better place to document it?
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.
@driusan the tables are all documented in the SQL patch included in this PR. I did not add any comments in 0000-00-00-schema.sql
since no table seems to be documented in there.
SQL/0000-00-00-schema.sql
Outdated
`MriProtocolID` INT(11) UNSIGNED NOT NULL, | ||
CONSTRAINT `FK_mri_protocol_group_rel_1` FOREIGN KEY (`MriProtocolGroupID`) REFERENCES `mri_protocol_group` (`MriProtocolGroupID`), | ||
CONSTRAINT `FK_mri_protocol_group_rel_2` FOREIGN KEY (`MriProtocolID`) REFERENCES `mri_protocol` (`ID`), | ||
CONSTRAINT `UK_mri_protocol_group_rel` UNIQUE (`MriProtocolID`) |
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.
If MriProtocolID is unique, then the MriProtocolGroupID should be added to the mri_protocol 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.
Ok. I'll remove table mri_protocol_group_rel
then.
getMRIProtocolData should return an array of MRIProtocolGroup or an array of array so that the template file could use imbricated foreach loops instead of adding all sort of template variable to define the limits of mri_protocol groups in the mri_protocol rows. |
oupss |
@nicolasbrossard |
SQL/0000-00-00-schema.sql
Outdated
`MriProtocolGroupID` INT(2) UNSIGNED NOT NULL AUTO_INCREMENT, | ||
`Name` VARCHAR(255) NOT NULL, | ||
PRIMARY KEY (`MriProtocolGroupID`) | ||
) ENGINE = InnoDB DEFAULT CHARSET=utf8; |
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.
) ENGINE = InnoDB DEFAULT CHARSET=utf8; | |
) ENGINE = InnoDB DEFAULT CHARSET=utf8mb4; |
SQL/0000-00-00-schema.sql
Outdated
CONSTRAINT `FK_mri_protocol_group_rel_1` FOREIGN KEY (`MriProtocolGroupID`) REFERENCES `mri_protocol_group` (`MriProtocolGroupID`), | ||
CONSTRAINT `FK_mri_protocol_group_rel_2` FOREIGN KEY (`MriProtocolID`) REFERENCES `mri_protocol` (`ID`), | ||
CONSTRAINT `UK_mri_protocol_group_rel` UNIQUE (`MriProtocolID`) | ||
) ENGINE = InnoDB DEFAULT CHARSET=utf8; |
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.
) ENGINE = InnoDB DEFAULT CHARSET=utf8; | |
) ENGINE = InnoDB DEFAULT CHARSET=utf8mb4; |
although in this case, it doesn't make a difference.
SQL/0000-00-00-schema.sql
Outdated
|
||
CREATE TABLE `mri_protocol_group_rel` ( | ||
`MriProtocolGroupID` INT(2) UNSIGNED NOT NULL, | ||
`MriProtocolID` INT(11) UNSIGNED NOT NULL, |
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.
Should have a UNIQUE
on MriProtocolID to go with the description below
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.
Agreed.
@@ -154,6 +154,7 @@ public function setUp() | |||
'series_description' => 'Test Description', | |||
'SeriesUID' => '5555', | |||
'TarchiveID' => '263', | |||
'MriProtocolGroupID' => 1, |
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.
The MriProtocolGroupID might not be 1 if the auto_increment is altered. I would be safer to get the Id first based on the Name. Unless you ignore the previous comment:
The
1
should be generated by the auto_increment. [...]_Originally posted by @xlecours in https://github.com/aces/Loris/pull/4321/files#diff-49c955f1af7f1ad35cdd7a9aceb590d1R811
@nicolasbrossard whats the status of this PR ? |
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.
Requesting changes so this doesn't show up in "Approved" and get accidentally merged.. please dismiss my review when the changes in the blocked PR are done.
60fdd06
to
9c05050
Compare
@nicolasbrossard please fix phan errors. |
@nicolasbrossard @kongtiaowang Travis still unhappy. |
Cecile says this is ready, so my pseudo-requested changes isn't relevant anymore
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'm not familiar with the MRI-side related code but I don't see any problems with the PHP/SQL.
Brief summary of changes
This PR adds support for multiple MRI protocols in LORIS. Each MRI protocol will have an associated set of lines in table
mri_protocol
that will define how scan types are determined for that protocol. Also, each protocol will have an associated set of conditions that specify under which circumstances it is to be used.The ability to group protocol checks together has also been implemented in a similar way.
The bulk of the changes related to this new feature are in the LORIS-MRI codebase. This PR modifies the following LORIS pages:
MRI protocol check violations: adds a column for the name of the protocol checks group to the result table
MRI protocol violations: adds a column for the name of the protocol group for both the result table and the MRI protocol table.
This resolves issue...
To test this change...
You must first source the SQL patch in this PR. You can afterwards insert dummy data in the
mri_protocol_group*
,mri_protocol_checks_group*
,mri_violations_log
andmri_protocol_violated_scans
and verify that the MRI violations module pages all display properly.Corresponding PR on the LORIS-MRI side is #369