Updated rules to set the default project, cohort #809

laemtl · 2022-10-19T17:34:41Z

Solves several problems:

Profile getSubjectIDs comments/documentation for $subjectID{'ProjectID'} updated
Candidate insert statement updated with RegistrationProjectID instead of ProjectID.
Adds new LORIS Configs: CreateVisit (replace the variable in the prod file), default_project and default_cohort (fallback to the variables in the prod file if not set). See createVisit/Project/Cohort imaging config Loris#8384
Updated rules to set the default project:
- Value in the prod file
- session ProjectID
- candidate RegistrationProjectID
- config default_project
- throw an error
Updated rules to set the default cohort:
- Value in the prod file
- session CohortID
- config default_cohort
- ("UNKN", 0)

nicolasbrossard · 2022-10-19T20:01:41Z

dicom-archive/profileTemplate.pl

@@ -88,6 +91,7 @@ sub getSubjectIDs {
        #     ? 1 : 2;
        # When createVisitLabel is set to 0, $subjectID{'SubprojectID'} is ignored.

+        $subjectID{'ProjectID'} = 1;


I am not sure I would give a default value to the ProjectID. I think it would be better to leave it undefined and have the MRI pipeline complain that if was not set instead of setting the project ID of the newly created candidate to 1 (which might not be the correct value). @cmadjar Can you weigh in on this?

The rest of the PR gets my approval.

I agree with @nicolasbrossard. Since ProjectID can be NULL I would leave the default to be undef.

Yeah I agree it's not a super safe change. I get confused by this change.
Since v24.0 session ProjectID cannot be null. createVisitLabel is set to 1 by default, so I suspect setting Project to undef will trigger an error when attempting to create a session . Is setting createVisitLabel to 0 by default a better option?

I am more worried about the scanner candidate having to have a ProjectID than a phantom session since that could technically be in the PatientName the same way the site is specified. But creating a scanner candidate for a specific project seems a little odd. Maybe we could create a 'scanner' project?

cmadjar · 2022-10-24T15:00:14Z

dicom-archive/profileTemplate.pl

@@ -79,6 +81,7 @@ sub getSubjectIDs {

        $subjectID{'createVisitLabel'} = 0;

+        # $subjectID{'SubprojectID'} = 1;


here as well, I would put the default to be undef

cmadjar · 2022-10-24T15:00:23Z

dicom-archive/profileTemplate.pl

        # When createVisitLabel is set to 1, SubprojectID must also
        # be set to the ID of the subproject that the newly created
        # visit should have. Assuming for example that all patient
        # names end with "_<mySubProjectID>", then we could write:
        # ($subjectID{'SubprojectID'}) = $patientName =~ /_(\d+)$/;
        # When createVisitLabel is set to 0, $subjectID{'SubprojectID'} is ignored.

+        # $subjectID{'ProjectID'} = 1;


here as well, I would put the default to be undef

cmadjar · 2022-10-24T15:00:26Z

dicom-archive/profileTemplate.pl

@@ -51,13 +51,15 @@ sub getSubjectIDs {

        $subjectID{'createVisitLabel'} = 1;

+        # $subjectID{'SubprojectID'} = 1;


here as well, I would put the default to be undef

cmadjar · 2022-10-24T15:05:29Z

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

+        DoB                   => $subjectIDsref->{'PatientDoB'},
+        Sex                   => $sex,
+        RegistrationCenterID  => $centerID,
+        RegistrationProjectID => $subjectIDsref->{'ProjectID'},


Given that the default ProjectID would be undef, I wonder whether the if statement line 1758 should not be modified by !defined $subjectIDsref->{'ProjectID'} and $pscid eq 'scanner' since no ProjectID should be assigned to scanner candidates.

@laemtl thoughts?

I believe the change here will not allow scanners to be registered without a ProjectID.

oh noooo. Indeed. I thought RegistrationProjectID could be NULL. Not sure what to do cause like you mentioned, a scanner can belong to more than one Project... Let's add that to tomorrow's LORIS agenda and see what we can do.

cmadjar · 2022-11-01T15:50:32Z

@laemtl Based on today's discussion at the LORIS call, the best approach to this might be to set the RegistrationProjectID of the scanner candidate to be created to the same RegistrationProjectID as the one used for the candidate being scanned (PatientName).

Then, when Phantom session are being inserted into LORIS, the session ProjectID will be dependent on the string in the PatientName (the project info will be in the PatientName the same way the site info is).

cmadjar · 2022-11-01T15:58:11Z

I guess, the same way we have determinePSC called before determineScannerID in tarchive_validation.pl, minc_insertion.pl and tarchiveLoader.pl, there should be a determineProjectID based on the PatientName which will then be passed to determineScannerID function the same way it is passed the CenterID.

@laemtl @nicolasbrossard would that make sense to you?

nicolasbrossard · 2022-11-07T20:04:58Z

@cmadjar Makes sense to me.

cmadjar · 2023-01-20T15:32:41Z

@laemtl ping on this one :)

cmadjar · 2023-03-02T22:04:05Z

@laemtl I tested the PR with the new config in the config module but I get the following error:

ERROR: Cannot create candidate 966666/ROM999 as the profile file does not define a ProjectID for him/her.

Looking at the code, I realize that the config values from the Config module are not being fetched as far as I can tell. Maybe you did not push the latest version of the code?

Note to myself for upcoming testing once this PR is updated to resume testing: (test on ROM666_966666_V1 which does not exist in RB with default Project set to Challah and default Cohort set to Stale)

tarchiveLoader.pl -profile prod -uploadID 124 /data/demo/data/tarchive/DCM_2016-08-19_ImagingUpload-16-50-H_7N05.tar

cmadjar · 2023-03-03T16:14:07Z

uploadNeuroDB/NeuroDB/objectBroker/ConfigOB.pm

@@ -93,6 +93,9 @@ use constant BIDS_DATASET_AUTHORS        => 'bids_dataset_authors';
 use constant BIDS_ACKNOWLEDGMENTS_TEXT   => 'bids_acknowledgments_text';
 use constant BIDS_README_TEXT            => 'bids_readme_text';
 use constant BIDS_VALIDATOR_OPTIONS_TO_IGNORE => 'bids_validator_options_to_ignore';
+use constant CREATE_CANDIDATES           => 'createCandidates';
+use constant CREATE_VISIT                => 'createVisit';
+use constant DEFAULT_COHORT              => 'default_cohort';


missing the following here:

Suggested change

use constant DEFAULT_COHORT => 'default_cohort';

use constant DEFAULT_COHORT => 'default_cohort';

use constant DEFAULT_PROJECT => 'default_project';

Which results in the following error:

Bareword "DEFAULT_PROJECT" not allowed while "strict subs" in use at /data/qpn/bin/mri/uploadNeuroDB/NeuroDB/objectBroker/ConfigOB.pm line 477.

Thanks, it was probably introduced by a merge conflict not properly resolved 😄

@cmadjar I will retest to make sure something else was not broken

laemtl · 2023-03-06T15:02:32Z

@cmadjar Ready!

cmadjar

LGTM

* ProjectID fix * New Config CreateVisit, default_project and default_cohort * Create getDefaultCohort and getDefaultProject in ConfigOB * Documentation and updates

laemtl force-pushed the ProjectID-fix branch from d2f9afa to fe57819 Compare October 19, 2022 18:36

laemtl requested review from nicolasbrossard and cmadjar October 19, 2022 18:36

nicolasbrossard reviewed Oct 19, 2022

View reviewed changes

cmadjar reviewed Oct 24, 2022

View reviewed changes

cmadjar requested changes Oct 24, 2022

View reviewed changes

cmadjar added this to the 25.0.0 milestone Feb 9, 2023

laemtl requested a review from cmadjar February 19, 2023 08:49

laemtl force-pushed the ProjectID-fix branch from afb670d to 7489f42 Compare February 23, 2023 20:53

laemtl force-pushed the ProjectID-fix branch from 7489f42 to afb670d Compare March 3, 2023 01:09

laemtl added 2 commits March 3, 2023 10:11

ProjectID fix

834d56b

New Config CreateVisit, default_project and default_cohort

516b093

laemtl force-pushed the ProjectID-fix branch from afb670d to 797eb1e Compare March 3, 2023 15:37

cmadjar reviewed Mar 3, 2023

View reviewed changes

Create getDefaultCohort and getDefaultProject in ConfigOB

c465f7d

laemtl force-pushed the ProjectID-fix branch 7 times, most recently from 070215b to 5f3f634 Compare March 6, 2023 08:06

Documentation and updates

a71b8af

laemtl force-pushed the ProjectID-fix branch from 5f3f634 to a71b8af Compare March 6, 2023 08:17

cmadjar approved these changes Mar 6, 2023

View reviewed changes

cmadjar mentioned this pull request Mar 6, 2023

createVisit/Project/Cohort imaging config aces/Loris#8384

Merged

cmadjar merged commit f20a3df into aces:main Mar 6, 2023

laemtl changed the title ~~ProjectID fix~~ Updated rules to set the default project, cohort Jul 10, 2023

zaliqarosli mentioned this pull request Sep 13, 2023

Add ProjectID to scanner candidate zaliqarosli/Loris-MRI#10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated rules to set the default project, cohort #809

Updated rules to set the default project, cohort #809

laemtl commented Oct 19, 2022 •

edited by cmadjar

Loading

nicolasbrossard Oct 19, 2022 •

edited

Loading

cmadjar Oct 24, 2022

laemtl Oct 27, 2022 •

edited

Loading

cmadjar Oct 31, 2022

cmadjar Oct 24, 2022

cmadjar Oct 24, 2022

cmadjar Oct 24, 2022

cmadjar Oct 24, 2022

laemtl Oct 27, 2022 •

edited

Loading

cmadjar Oct 31, 2022

cmadjar commented Nov 1, 2022

cmadjar commented Nov 1, 2022

nicolasbrossard commented Nov 7, 2022

cmadjar commented Jan 20, 2023

cmadjar commented Mar 2, 2023

cmadjar Mar 3, 2023

laemtl Mar 4, 2023

laemtl Mar 4, 2023

laemtl commented Mar 6, 2023

cmadjar left a comment

		@@ -79,6 +81,7 @@ sub getSubjectIDs {

		$subjectID{'createVisitLabel'} = 0;

		# $subjectID{'SubprojectID'} = 1;

		@@ -51,13 +51,15 @@ sub getSubjectIDs {

		$subjectID{'createVisitLabel'} = 1;

		# $subjectID{'SubprojectID'} = 1;

	use constant DEFAULT_COHORT => 'default_cohort';
	use constant DEFAULT_COHORT => 'default_cohort';
	use constant DEFAULT_PROJECT => 'default_project';

Updated rules to set the default project, cohort #809

Updated rules to set the default project, cohort #809

Conversation

laemtl commented Oct 19, 2022 • edited by cmadjar Loading

nicolasbrossard Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar commented Nov 1, 2022

cmadjar commented Nov 1, 2022

nicolasbrossard commented Nov 7, 2022

cmadjar commented Jan 20, 2023

cmadjar commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl commented Mar 6, 2023

cmadjar left a comment

Choose a reason for hiding this comment

laemtl commented Oct 19, 2022 •

edited by cmadjar

Loading

nicolasbrossard Oct 19, 2022 •

edited

Loading

laemtl Oct 27, 2022 •

edited

Loading

laemtl Oct 27, 2022 •

edited

Loading