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

[SQL] Removed mri_acquisition_dates table from the database and the LORIS code #4962

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Jul 17, 2019

Brief summary of changes

This PR removes the mri_acquisition_dates table from the database as this table does not necessarily store the correct acquisition date. Instead, replaced all the queries to look for the minimal date present in the parameter_file table which is more robust than what was stored in mri_acquisition_dates.

Note: the decision to remove this table was taken during an imaging meeting in 2017 and confirmed at an other imaging meeting in 2019.

Link to the MRI PR: aces/Loris-MRI#463

Testing instructions (if applicable)

  1. Run the SQL patch SQL/New_patches/2019-07-17_remove_mri_acquisition_dates_table.sql
  2. Make sure the dashboard is loading and plotting the correct graph for the MRI acquisitions
  3. Make sure the imaging browser is loading and that the First Acquisition date properly populated

Links to related tickets (GitHub, Redmine, ...)

@cmadjar cmadjar added Release: Add to release notes PR whose changes should be highlighted in the release notes Cleanup PR or issue introducing/requiring at least one clean-up operation Language: SQL PR or issue that update SQL code [branch] minor labels Jul 17, 2019
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 think this should go to major, as it's likely there are queries out there in the world that depend on it which would break that will need to be updated (ie. analysis pipelines that use it to get the date of the nearest phantom.)

@cmadjar cmadjar changed the base branch from minor to major July 17, 2019 14:48
@cmadjar cmadjar force-pushed the delete_mri_acquisition_dates_table branch from 2c78f05 to 49da3bf Compare July 17, 2019 14:50
@cmadjar
Copy link
Collaborator Author

cmadjar commented Jul 17, 2019

@driusan Done :)

@johnsaigle
Copy link
Contributor

> ./test/run-php-linter.sh
FILE: ...aces/Loris/modules/imaging_browser/php/imaging_browser.class.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 236 | WARNING | Line exceeds 85 characters; contains 88 characters
----------------------------------------------------------------------

Small code change needed to pass Travis

@johnsaigle johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Jul 17, 2019
@cmadjar cmadjar removed the State: Needs work PR awaiting additional work by the author to proceed label Jul 18, 2019
@cmadjar
Copy link
Collaborator Author

cmadjar commented Jul 18, 2019

@johnsaigle Thank you for the copy of the Travis error! It was very handy to not have to go through the whole log ;).

Should be fixed now and Travis will hopefully be happy this time!

Copy link
Contributor

@ycAbout ycAbout left a comment

Choose a reason for hiding this comment

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

Study progression dashboard is failing after switch to your branch for the same data set.
On 21.0 release:
Screenshot from 2019-07-22 14-58-13

On your branch (Before and after applying the SQL patch are the same):
Screenshot from 2019-07-22 15-00-52

The error might be in this file: modules/dashboard/ajax/get_scan_line_data.php

@cmadjar cmadjar added State: Needs work PR awaiting additional work by the author to proceed and removed State: Needs work PR awaiting additional work by the author to proceed labels Jul 23, 2019
@cmadjar
Copy link
Collaborator Author

cmadjar commented Jul 23, 2019

@coreyAbout It should be fixed now. I also fixed Travis (hopefully)

@ycAbout
Copy link
Contributor

ycAbout commented Jul 29, 2019

Great! It works! @cmadjar

@driusan driusan merged commit ddcf784 into aces:major Jul 30, 2019
@cmadjar cmadjar deleted the delete_mri_acquisition_dates_table branch July 30, 2019 20:02
@ridz1208 ridz1208 added this to the 22.0.0 milestone Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Language: SQL PR or issue that update SQL code Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants