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

Order the acquisitions per modality based on series number: Redmine 10959 #160

Merged
merged 5 commits into from
Oct 14, 2016

Conversation

MounaSafiHarab
Copy link
Contributor

@MounaSafiHarab MounaSafiHarab commented Sep 28, 2016

Order the files with the same AcquisitionProtocolID (within a tarchiveID) by series_number.
aces/Loris#2221

@cmadjar
Copy link
Collaborator

cmadjar commented Oct 7, 2016

Is there a script or patch that can be run on files that were already inserted into the DB?

# load the file object to get the series_number
while (my $row = $acqArr->fetchrow_hashref()) {
$acqProtID = $row->{'AcquisitionProtocolID'};
$query = "SELECT f.FileID, f.ModalityOrder ".
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any 'ModalityOrder' field in the files table. Am I misunderstanding that query? Unless it is meant to be the new field but in the loris pull request it has been named 'AcqOrderPerModality'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, you understood it right. I misnamed it here (it was the original name I chose but then I updated it in Loris but forgot to update here). Thanks for the catch!
done now

my $query = "SELECT DISTINCT f.AcquisitionProtocolID ".
"from files f ";
my $where = "WHERE f.TarchiveSource=?";
$query = $query . $where;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The $where variable is not useful. I would write the whole query directly in the $query variable.
Also, FROM was not capitalized here (not crucial but to stay consistent)

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.

$query = "SELECT f.FileID, f.ModalityOrder ".
"from files f ";
$where = "WHERE f.TarchiveSource=? AND f.AcquisitionProtocolID=?";
$query = $query . $where;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. Get rid of the $where variable and capitalize "FROM"

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

$dataArr = ${$this->{'dbhr'}}->prepare($query);
$dataArr->execute($tarchiveID, $acqProtID);
my (@fileID, @seriesNumber)=();
my ($fileID, $seriesNumber)=();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this line repeated with @ (array) and $ (dereferenced array)?

my ($fileID, $seriesNumber)=();
my $i=0;
while (my $row2 = $dataArr->fetchrow_hashref()) {
$i++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Incrementation should be put at the end of the loop as arrays start at index 0.


my $order = 0;
foreach my $j (0..$#seriesNumber-1) {
$order++;
Copy link
Collaborator

@cmadjar cmadjar Oct 7, 2016

Choose a reason for hiding this comment

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

Will need to move incrementation at the end too.

$order++;
$query = "UPDATE files f SET f.ModalityOrder=? ";
$where = "WHERE f.FileID=?";
$query = $query . $where;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as before, the $where variable is not useful here. Remove it and write the query in one shot.

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

my $modalityOrder_update = ${$this->{'dbhr'}}->prepare($query);
$modalityOrder_update->execute($order, $sorted_fileID[$order]);
$message = "The Modality Order for FileID $sorted_fileID[$order] was updated to $order \n ";
print "Message is: $message \n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

get rid of the print statement here and keep it only for the LOGs

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

my $modalityOrder_update = ${$this->{'dbhr'}}->prepare($query);
$modalityOrder_update->execute($order, $sorted_fileID[$order]);
$message = "The Modality Order for FileID $sorted_fileID[$order] was updated to $order \n ";
$this->{LOG}->print($message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

alignment issue

@MounaSafiHarab
Copy link
Contributor Author

@cmadjar
I took care of the rest of the comments. It should be ready if you want to test it again.

re. the comment on populating existing data; I will need to generate a script. I am not sure this is doable through a simple mysql patch.

@cmadjar
Copy link
Collaborator

cmadjar commented Oct 13, 2016

Works like a charm :)
screen shot 2016-10-13 at 10 39 32 am

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.

2 participants