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

Quality control fixes and enhancements #1851

Merged
merged 74 commits into from
Dec 12, 2023

Conversation

julien-louis
Copy link
Collaborator

No description provided.

@michaelkain michaelkain self-requested a review September 22, 2023 06:43
@julien-louis julien-louis changed the title Quality by exam, with progress bar Quality control fixes and enhancements Nov 16, 2023
@julien-louis
Copy link
Collaborator Author

done

@julien-louis
Copy link
Collaborator Author

  • Applying qc on large study (**AN), on qualif : 45 min

    • Only testing on the whole study : 20 min ???
  • with parallelization : Test = 11min

Copy link
Contributor

@michaelkain michaelkain left a comment

Choose a reason for hiding this comment

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

THX, fine, code reviewed again

@michaelkain
Copy link
Contributor

michaelkain commented Dec 8, 2023

Hi Julien, I hope you are doing fine. My tests on qualif, when I use apply, do not produce any tags in the tree. Could you please as soon as possible have a look on that? When I click on the task detail in the jobs list, I do not have access to the quality report. Is this wanted?

@michaelkain
Copy link
Contributor

michaelkain commented Dec 8, 2023

Thank you for your fast answer, you are completely right, my conditions where too strict and no exam was valid, that is why I did not have a tag.

@michaelkain
Copy link
Contributor

Hi @julien-louis, thank you for the stack trace. As discussed, please fix the NPE related to org.shanoir.ng.shared.event.ShanoirEvent["report"]), that blocks imports. Thank you and with kind regards, Michael

@michaelkain
Copy link
Contributor

michaelkain commented Dec 8, 2023

Hi Julien, please see my screenshot attached.
Screenshot from 2023-12-08 17-10-15
Do you agree, that my rule means all datasets of the exam must be t1, and if not, throw an error?
It does not work. I could import the sample.zip, that contains only not-t1 series and I had no error.
I used an existing exam, if this helps, but anyway, with the import, we only check the dicom study, that is currently imported?

@julien-louis
Copy link
Collaborator Author

I couldn't import sample.zip in my new empty study test julien 2 :
image

qc :
image

@michaelkain
Copy link
Contributor

have you tested with an existing exam, or a new created one?

@michaelkain
Copy link
Contributor

michaelkain commented Dec 8, 2023

I tested with an existing exam, that had already only t1 inside, so I took an exam, I had created during my previous import. I would expect the QC to check only my DICOM study I am about to import, and not the already imported exam

@michaelkain
Copy link
Contributor

Screenshot from 2023-12-08 17-27-44
We can not go to prod with the above error. We have many, many empty study cards. And an empty study card is not an error.

@julien-louis
Copy link
Collaborator Author

@michaelkain actually the empty study card message was about empty quality cards, I changed the message. I don't know what to do with an empty quality card. Return an empty report ?

@julien-louis
Copy link
Collaborator Author

@michaelkain Ok I checked and you are right, I think at import the quality control is on the whole exam, not just the imported data. I will assert that immediately.

}
}
return qualityResult;
}

private QualityCardResult checkQuality(Examination examination, Set<DatasetAcquisition> limitToTheseAcquisitions, ImportJob importJob) throws ShanoirException {
// save the exam acquisitions
List<DatasetAcquisition> saveList = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Julien, thank you for your corrections. I am wondering about the below code. The ImporterService is only responsible for new-imported-data (either on a new and empty exam or an existing exam). The below code is really quite a work-around (sorry for the word), but putting existing acquisitions into memory, simulate a new exam and then putting back the old acquisitions, is really strange. I have fear in two months we will not understand the code ourselves. Please let's discuss this and how we continue from here. Thank you and with kind regards, Michael

@michaelkain
Copy link
Contributor

Hi, I have a new issue during my tests. The dates are corrupted somehow in the report table in the frontend and in the csv, please see my screenshot. With kind regards, Michael
Screenshot from 2023-12-12 11-38-57

@michaelkain
Copy link
Contributor

Thank you for the time added to the tasks table! Very useful!

Copy link
Contributor

@michaelkain michaelkain left a comment

Choose a reason for hiding this comment

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

Hi Julien,
my tests on qualif are looking fine now. We will plan the refactor for 2024, see my last comment. I approve. See below my test result, with kind regards, Michael

QC rule created, on study demonstration:
Quality rule, that requires at least one T1,t1 serie in the metadata of shanoir

TEST: test button used: correct results, apply button used: correct results: OK

--- IMPORT-TESTS:

TEST: import quality conform serie, into new subject, new exam: OK, import worked
t1 phantom serie used, new subject + exam created during import

TEST: use quality rule conform serie, into existing exam, that is not quality-conform: OK, import worked
1 serie selected, that is t1_fl2d_tra_3mm mtc 10/08/2009 MR
Use subject TestMK20210107-01, that has existing exam CT chest plan

TEST: use serie, not quality-conform, into existing exam, that is quality-conform: OK, import refused
1 serie selected tse_vfl_WIP607 04/03/2010 MR, not qc conform
into existing exam selected with one serie, qc conform

@michaelkain michaelkain merged commit 76993d9 into fli-iam:develop Dec 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants