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

Series filtering out based on description string #351

Merged
merged 34 commits into from
Apr 10, 2024
Merged
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
a8aa826
Move placeholder value to central location
jeremyestein Mar 20, 2024
397d658
Return proper HTTP errors inside orthanc raw "/send-to-anon" API call
jeremyestein Mar 21, 2024
ab6371c
Check error code from shell - should use requests API but never mind
jeremyestein Mar 21, 2024
589ee29
Refactor waiting for query into orthanc code
jeremyestein Mar 22, 2024
855f371
Merge branch 'main' into jeremy/series-filtering
jeremyestein Mar 22, 2024
2aea37b
The "correct" test data wasn't being validated to ensure it was actually
jeremyestein Mar 22, 2024
0efb732
Merge branch 'jeremy/config-fixes' into jeremy/series-filtering
jeremyestein Mar 22, 2024
a894f13
Remove unnecessary try except
jeremyestein Mar 25, 2024
1916a3a
Merge branch 'main' into jeremy/series-filtering
jeremyestein Mar 25, 2024
61520d5
Add explanatory comments
jeremyestein Mar 27, 2024
31d34f5
Move private tag creation code to library
jeremyestein Mar 27, 2024
3d1d7ba
Merge branch 'main' into jeremy/series-filtering
jeremyestein Mar 27, 2024
a0afd82
Merge branch 'main' into jeremy/series-filtering
jeremyestein Mar 27, 2024
de4356b
Reuse existing method
jeremyestein Mar 27, 2024
1997d6a
Add series filtering feature and some unit tests (system tests to come)
jeremyestein Mar 27, 2024
958e3b6
Speculative fix for occasional rabbitmq crashes
jeremyestein Mar 27, 2024
be7e094
I think this check got missed
jeremyestein Mar 27, 2024
0ce17ca
First attempt at a system test. The data is not behaving though.
jeremyestein Mar 27, 2024
706c4d4
The fake VNA was not seeing the test instances as part of the same
jeremyestein Mar 28, 2024
a6f5a85
Modality was causing some test instances to get filtered out. Also de…
jeremyestein Mar 28, 2024
684855a
Tidy up system test and move dicom data set up from shell script to
jeremyestein Apr 9, 2024
6702a3d
Merge branch 'main' into jeremy/series-filtering
jeremyestein Apr 9, 2024
4a05bf5
Fix the merge; de-dupe the VR field
jeremyestein Apr 10, 2024
a2a1f51
Some typing fixes needed now that we have mypy enabled
jeremyestein Apr 10, 2024
6625174
Merge branch 'main' into jeremy/series-filtering
jeremyestein Apr 10, 2024
a844cee
Pytest classes cannot have an __init__ method; several tests were being
jeremyestein Apr 10, 2024
6e4b75a
Merge branch 'jeremy/fix-system-test-class' into jeremy/series-filtering
jeremyestein Apr 10, 2024
ad4aaa4
Return accurate content type
jeremyestein Apr 10, 2024
493fd66
Make series description test case-insensitive
jeremyestein Apr 10, 2024
21f7f1b
Remove defunct comment
jeremyestein Apr 10, 2024
7e9bd41
Tidy up logging
jeremyestein Apr 10, 2024
0eec011
Merge branch 'main' into jeremy/series-filtering
jeremyestein Apr 10, 2024
debf08c
Get the media type actually right this time.
jeremyestein Apr 10, 2024
2d71237
Move method to utility module
jeremyestein Apr 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Tidy up system test and move dicom data set up from shell script to
python, losing Dicom2.dcm altogether, at least for now
jeremyestein committed Apr 9, 2024
commit 684855a5a28fbe6b34bdcb48c7245c9fc7ae9d28
28 changes: 15 additions & 13 deletions test/conftest.py
Original file line number Diff line number Diff line change
@@ -54,10 +54,6 @@ def _populate_vna(tmp_path_factory) -> None:
dicom_dir = tmp_path_factory.mktemp("dicom_series")
# more detailed series testing is found in pixl_dcmd tests, but here
# we just stick an instance to each study, one of which is expected to be propagated through
# Move VNA population to here from insert_test_data.sh so it's all in one place?
acc_num_1 = "AA12345601"
acc_num_2 = "AA12345605"
patient_id_1 = "987654321"

# studies are also defined by the StudyID, the StudyInstanceUID
def study_instance_uid(offset: int) -> str:
@@ -76,29 +72,34 @@ def sop_instance_uid(offset: int) -> str:
return dict(SOPInstanceUID=baseline[: -len(offset_str)] + offset_str)

study_1 = dict(
AccessionNumber=acc_num_1,
PatientID=patient_id_1,
AccessionNumber="AA12345601",
PatientID="987654321",
StudyID="12340001",
**study_instance_uid(1),
)
study_2 = dict(
AccessionNumber=acc_num_2,
PatientID=patient_id_1,
AccessionNumber="AA12345605",
PatientID="987654321",
StudyID="12340002",
**study_instance_uid(2),
)

# series are also defined by the SeriesInstanceUID, SeriesNumber
# SeriesNumber doesn't have to be gloablly unique, only within a study,
# Series are also defined by the SeriesInstanceUID and SeriesNumber.
# SeriesNumber doesn't have to be globally unique, only within a study,
# however I'm assuming SeriesInstanceUID does. So that must be generated when
# a series is attached to a study
series_1 = dict(SeriesDescription="whatever1", SeriesNumber=901, Modality="DX")
# a series is attached to a study.
series_0 = dict(SeriesDescription="AP", SeriesNumber=900, Modality="DX")
series_1 = dict(SeriesDescription="include123", SeriesNumber=901, Modality="DX")
# excluded by modality filter
series_exclude_2 = dict(SeriesDescription="whatever2", SeriesNumber=902, Modality="MR")
series_exclude_2 = dict(SeriesDescription="exclude123", SeriesNumber=902, Modality="MR")
# excluded by series description
series_exclude_3 = dict(SeriesDescription="positioning", SeriesNumber=903, Modality="DX")

# instances are also defined by the SOPInstanceUID

# to replace Dicom1.dcm
instance_0 = dict(**study_1, **series_0, **series_instance_uid(0), **sop_instance_uid(0))

instance_1 = dict(**study_1, **series_1, **series_instance_uid(1), **sop_instance_uid(1))
instance_2 = dict(
**study_1, **series_exclude_2, **series_instance_uid(2), **sop_instance_uid(2)
@@ -111,6 +112,7 @@ def sop_instance_uid(offset: int) -> str:
**study_2, **series_exclude_3, **series_instance_uid(5), **sop_instance_uid(5)
)

_upload_dicom_instance(dicom_dir, **instance_0)
_upload_dicom_instance(dicom_dir, **instance_1)
_upload_dicom_instance(dicom_dir, **instance_2)
_upload_dicom_instance(dicom_dir, **instance_3)
8 changes: 0 additions & 8 deletions test/scripts/insert_test_data.sh
Original file line number Diff line number Diff line change
@@ -38,11 +38,3 @@ insert into star.lab_result(lab_result_id, lab_order_id, lab_test_definition_id,
"

docker exec system-test-fake-star-db /bin/bash -c "psql -U postgres -d emap -c \"$_sql_command\""

# Uses an accession number of "AA12345601" for MRN 987654321
curl -X POST -u "orthanc:orthanc" "http://localhost:8043/instances" \
--data-binary @"$SCRIPT_DIR/../resources/Dicom1.dcm"
# Uses an accession number of "AA12345605" for MRN 987654321, already has project name added
# Send to orthanc raw to ensure that we can resend an existing message without querying VNA again
curl -X POST -u "orthanc_raw_username:orthanc_raw_password" "http://localhost:7005/instances" \
--data-binary @"$SCRIPT_DIR/../resources/Dicom2.dcm"
29 changes: 13 additions & 16 deletions test/system_test.py
Original file line number Diff line number Diff line change
@@ -83,15 +83,16 @@ def two_zip_files_present() -> bool:
progress_string_fn=zip_file_list,
)
expected_studies = {
"a971b114b9133c81c03fb88c6a958f7d95eb1387f04c17ad7ff9ba7cf684c392": [
"a971b114b9133c81c03fb88c6a958f7d95eb1387f04c17ad7ff9ba7cf684c392": {
# tuple made up of (AccessionNumber, SeriesDescription)
# hash of AA12345601
{"AccessionNumber": "ad630a8a84d72d71", "SeriesDescription": "whatever1"},
{"AccessionNumber": "ad630a8a84d72d71", "SeriesDescription": "AP"},
],
"f71b228fa97d6c87db751e0bb35605fd9d4c1274834be4bc4bb0923ab8029b2a": [
("ad630a8a84d72d71", "include123"),
("ad630a8a84d72d71", "AP"), # this is not found!!
},
"f71b228fa97d6c87db751e0bb35605fd9d4c1274834be4bc4bb0923ab8029b2a": {
# hash of AA12345605,
{"AccessionNumber": "c2f4b59b0291c6fe", "SeriesDescription": "whatever1"},
],
("c2f4b59b0291c6fe", "include123"),
},
}
assert zip_files
for z in zip_files:
@@ -110,17 +111,13 @@ def _check_dcm_tags_from_zip(self, zip_path, unzip_dir, expected_studies) -> Non
# One zip file == one study.
# There can be multiple instances in the zip file, one per file
logging.info("JES: within zip, dicom_in_zip [%s] = %s", len(dicom_in_zip), dicom_in_zip)
actual_instances = set()
for dcm_file in dicom_in_zip:
dcm = pydicom.dcmread(dcm_file)
# The actual dicom filename and dir structure isn't checked - should it be?
logging.info("JES: dcm_file = %s", dcm_file)
assert dcm.get("PatientID") == zip_path.stem # PatientID stores study id post anon
actual_info = {
"AccessionNumber": dcm.get("AccessionNumber"),
"SeriesDescription": dcm.get("SeriesDescription"),
}
logging.info("Actual info = %s", actual_info)
assert actual_info in expected_instances
actual_instances.add((dcm.get("AccessionNumber"), dcm.get("SeriesDescription")))
block = dcm.private_block(
DICOM_TAG_PROJECT_NAME.group_id, DICOM_TAG_PROJECT_NAME.creator_string
)
@@ -132,13 +129,13 @@ def _check_dcm_tags_from_zip(self, zip_path, unzip_dir, expected_studies) -> Non
# See https://github.com/UCLH-Foundry/PIXL/issues/363
logging.error(
"TEMPORARILY IGNORE: tag value %s should be of type str, but is of type bytes",
{private_tag.value},
private_tag.value,
)
assert private_tag.value.decode() == TestFtpsUpload.project_slug
else:
assert private_tag.value == TestFtpsUpload.project_slug
# nothing got left out
assert len(dicom_in_zip) == len(expected_instances)
# check the basic info about the instances exactly matches
assert actual_instances == expected_instances


@pytest.mark.usefixtures("_setup_pixl_cli")