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

Imaging Preprocessing Scripts: Loris-MRI configurations moved to the front end from the $profile file: Redmine 8893 #2649

Merged
merged 13 commits into from
Aug 1, 2017

Conversation

MounaSafiHarab
Copy link
Contributor

@MounaSafiHarab MounaSafiHarab commented Mar 13, 2017

  • Imaging insertion scripts configuration options can now be set from the Configuration module and/or Config table in the database, instead of the $profile ("prod") file in the Loris-MRI repo.

  • Please check the Loris-MRI pull request (Loris-MRI configurations moved to the front end from the $profile file: Redmine 8893 Loris-MRI#182) for existing projects on how to migrate existing $profile values to the database.

  • These settings can not be overwritten in the config.xml (because they are used by Loris-MRI perl scripts)

Related to:
aces/Loris-MRI#182
and
aces/dicom-archive-tools#45

@MounaSafiHarab MounaSafiHarab added State: Needs work PR awaiting additional work by the author to proceed SQL PR contains SQL modifications such as schema changes and new SQL scripts labels Mar 13, 2017
@MounaSafiHarab MounaSafiHarab added this to the 17.1 milestone Mar 13, 2017
@MounaSafiHarab
Copy link
Contributor Author

@driusan
default values are set as if it is a new install (so I took the current default values in $profileTemplate);
is it Ok to assume that projects will import the values from their prod file? (since I assume I have no way of reading their "prod" file)

@ridz1208 ridz1208 requested a review from driusan March 14, 2017 01:33
@driusan
Copy link
Collaborator

driusan commented Mar 14, 2017

@MounaSafiHarab Interesting question. I think you're right that we don't have any way to extract them from the prod file (unless you write a script on the Loris-MRI side), so I don't think there's really anything else you could do than just use the defaults.

Is there anything in them that's going to affect the data validity if people forget to manually change them in the config module?

@MounaSafiHarab MounaSafiHarab removed the State: Needs work PR awaiting additional work by the author to proceed label Mar 14, 2017
@gluneau
Copy link
Contributor

gluneau commented Mar 27, 2017

Data truncated for column 'Description'

INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'converter', 'If converter is set to dcm2mnc, the c-version of dcm2mnc will be used. If however you want to use any of the various versions of the perl converter dicom_to_minc of mydcm2mnc you will have to specify what it is called and the uploader will try to invoke it', 1, 0, 'text', ID, 'dcm2mnc binary to use when converting', 1 FROM ConfigSettings WHERE Name="imaging_pipeline"

1 row(s) affected, 1 warning(s): 1265 Data truncated for column 'Description' at row 1 Records: 1  Duplicates: 0  Warnings: 1

image

@MounaSafiHarab
Copy link
Contributor Author

@driusan
I added the script to read the values from the $profile to change those "default" values on the Loris-MRI side. It is added to my Loris-MRI pull request, and it is placed in the tools/ directory of the Loris-MRI codebase.

@gluneau
I addressed the truncated value.

@driusan
Copy link
Collaborator

driusan commented Apr 3, 2017

@MounaSafiHarab this has conflicts that need to be resolved.

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.

I assume that the default value's that it's using are the same as what's currently in the default pofileTemplate, in which case it looks fine.. but it needs to be merged alongside the script in Loris-MRI that populates it for existing projects, and the PR that updates the code to use the config setting for the imaging pipeline..

@driusan
Copy link
Collaborator

driusan commented Apr 7, 2017

also, when this is eventually merged it needs to be squashed to ensure that the unnecessary merge commit doesn't get committed into the history.

@MounaSafiHarab
Copy link
Contributor Author

@driusan
yes the default values in the Loris-MRI installer were used here as well.
I agree, this should NOT be merged before the Loris-MRI one is approved and about to be merged!

@driusan driusan added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label May 30, 2017
@ridz1208
Copy link
Collaborator

ridz1208 commented Jun 8, 2017

@MounaSafiHarab does this still need testing or is it ready to merge after the related PR has been merged ?

@MounaSafiHarab
Copy link
Contributor Author

MounaSafiHarab commented Jun 8, 2017

@ridz1208

This needs testing mostly on the Loris-MRI side (main changes are there not in this patch) which is why I believe @driusan added the Blocked label. @gluneau was supposed to test it when he is back.

@driusan
Copy link
Collaborator

driusan commented Jun 8, 2017

Yeah, I added the blocked tag because of the 3 interrelated pull requests.

Maybe they can be discussed/merged at an imaging meeting to make sure they go in in the right order and all works together?

@MounaSafiHarab
Copy link
Contributor Author

@cmadjar
as discussed in the imaging meeting today, assigned to you, along with the Loris-MRI ones (specifically to test the DTI pipeline), while @gluneau is focusing on the /data/ being on a different VM (mounted) as is the case for IBIS

@MounaSafiHarab
Copy link
Contributor Author

@cmadjar
@gluneau

please test at your earliest convenience (i.e. ASAP:)) so I can incorporate your feedback!



-- default imaging_pipeline settings
INSERT INTO Config (ConfigID, Value) SELECT ID, "/PATH/TO/DATA/location" FROM ConfigSettings cs WHERE cs.Name="Loris-MRI Data Directory";
Copy link
Contributor

Choose a reason for hiding this comment

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

cs.Name should be: "dataDirBasepath"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gluneau

thanks!

@gluneau
Copy link
Contributor

gluneau commented Jul 20, 2017

@MounaSafiHarab @driusan tested with IBIS data, and with insertion of a zip.

@samirdas samirdas merged commit d96c455 into aces:17.1-dev Aug 1, 2017
@driusan driusan removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SQL PR contains SQL modifications such as schema changes and new SQL scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants