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

[post 19.0.0 release] loris-mri-docs: forward changes from v19.0.0 and resolve conflicts #280

Merged
merged 154 commits into from
Feb 28, 2018

Conversation

MounaSafiHarab
Copy link
Contributor

No description provided.

driusan and others added 30 commits March 30, 2011 16:14
Add incoming site as config option.
Fixed the dcmodify --insert-tag option to --insert
…cations

Modification to the profileTemplate to be able to use DTIPrep pipeline
… object.

Other fails while reading content of dcm source directory
Modified $d to $dir in profileTemplate
lookupCenterID using patient name instead of PatientID
MounaSafiHarab and others added 5 commits January 24, 2018 10:10
…le (aces#260)

* Better MINC toolkit sourcing command and updated VERSION file

* cecile feedback
…FileID to NULL in files_qcstatus and feedback_mri_comments (aces#261)
…hs (Redmine 13915) (aces#272)

* Populate Loris-MRI code path for new installs since the user is asked to enter the MRI project name

* Populate other MRI data path with a better option than the default LORIS %PROJECTNAME

* post installation checks

* Add the condition Dave brought up during the imaging meeting; i.e. only change it for those that did not do it in the front end already

* Ceciles feedback

* oopsie
@MounaSafiHarab MounaSafiHarab changed the title loris-mri-docs: forward changes from v19.0.0 and resolve conflicts [post 19.0.0 release] loris-mri-docs: forward changes from v19.0.0 and resolve conflicts Feb 23, 2018
@MounaSafiHarab
Copy link
Contributor Author

@nicolasbrossard @cmadjar
this one was tricky,I had to merge changes and resolve conflicts manually...
please take a look carefully (idea is to have all our perl documentation changes WHILE incoporating all the code changes we did for 19.0-dev such as Cecile's tarchiveID to violated scans, Nick's float comparison, etc...)

Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@MounaSafiHarab I ran the scripts and noticed 2 things:

  1. the getConfigSetting subroutine is present twice in DBI.pm (one with the documentation and one without the documentation)

  2. I get this warning when running the tarchiveLoader script:
    Illegal character in prototype for main::logHeader : $date,$tarchive,$TmpDir at tarchiveLoader line 767
    I dug a little bit more and noticed that in your branch, the function logHeader () has 3 arguments ($date,$tarchive,$TmpDir) in line 767 but is called without any argument at line 234. I looked into 19.0-dev and saw that the function is created without arguments at line 767.
    Not sure I understand why I don't see this in the diff.

Let me know if it is unclear

@@ -65,6 +65,22 @@ sub connect_to_db
return $dbh;
}

sub getConfigSetting
Copy link
Collaborator

Choose a reason for hiding this comment

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

The getConfigSetting function is present twice in the DBI.pm file (one with the documentation and one without). Remove the one without the documentation.

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, good catch!

@MounaSafiHarab
Copy link
Contributor Author

MounaSafiHarab commented Feb 26, 2018

@MounaSafiHarab I ran the scripts and noticed 2 things:

the getConfigSetting subroutine is present twice in DBI.pm (one with the documentation and one without the documentation)

I get this warning when running the tarchiveLoader script:
Illegal character in prototype for main::logHeader : $date,$tarchive,$TmpDir at tarchiveLoader line 767
I dug a little bit more and noticed that in your branch, the function logHeader () has 3 arguments ($date,$tarchive,$TmpDir) in line 767 but is called without any argument at line 234. I looked into 19.0-dev and saw that the function is created without arguments at line 767.
Not sure I understand why I don't see this in the diff.

Let me know if it is unclear

I fixed item 1)
For item 2); I was aware of this; remember my comments to your pull request here:
#237

But I think this should be fixed in the loris-mri-docs branch, not this pull request(issued here: #281). The arguments added to the function were in the loris-mri-docs branch and here all I am doing is to bring the 19.0-dev changes to the loris-mri-docs branch so it makes sense to have the funcrtion arguments (because they were on the loris-mri-docs branch)...

@MounaSafiHarab
Copy link
Contributor Author

@cmadjar
If PR#281 is merged first, I can merge the latest loris-mri-docs branch here...

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 26, 2018

@cmadjar
If PR#281 is merged first, I can merge the latest loris-mri-docs branch here...

@MounaSafiHarab #281 just got merged. Thank you!!

@MounaSafiHarab
Copy link
Contributor Author

@MounaSafiHarab #281 just got merged. Thank you!!

and I just merged the loris-mri-docs/ branch here. thanks for catching those :)!

Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

👍

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 27, 2018

@MounaSafiHarab Sorry, conflicts detected now that I merged #273 :S

@MounaSafiHarab
Copy link
Contributor Author

lol I expected it, it is fine, I am just happy we are moving forward with these pull requests, thanks to you!

@MounaSafiHarab
Copy link
Contributor Author

@cmadjar
conflicts resolved

@cmadjar
Copy link
Collaborator

cmadjar commented Feb 27, 2018

@nicolasbrossard looks all good on my end. If you approve the PR as well, could you merge it at the same time? Thanks!!

@@ -0,0 +1,9 @@
### Example scripts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file should be removed as it was removed in loris-mri-docs/

@nicolasbrossard nicolasbrossard merged commit bb28206 into aces:loris-mri-docs Feb 28, 2018
@cmadjar cmadjar added this to the N/A milestone Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet