FilesystemDicomClient, studies_for_patient - Add support for partial date ranges #48
Conversation
- Fix / add support for partial date range filter strings, e.g. using `START-` or `-END` instead of `START-END`
joshuatz
force-pushed
the
filesystem-date-filtering
branch
from
b965ad2 to
9810e66
Compare
There was a problem hiding this comment.
Changes look good Josh!
I had a couple of minor comments and one test change request. Let me know if you have any questions.
Also, would you mind adding the -v (verbose) flag to the pytest invocations in tox.ini in this PR? There have been several pacsman PRs recently where it was unclear whether the newly added test was executed or not. If we use verbose, it will list all tests that were executed and we can just look at the logs to verify everything has been tested as expected.
Thanks for identifying this deficiency in filesystem dicom client and putting up a fix!
| @@ -144,10 +147,19 @@ def date_filter(study_ds): | |||
| else: | |||
| study_date_str = None | |||
|
|
|||
| if study_start_date is None or study_end_date is None or study_date_str is None: | |||
| if (study_start_date is None and study_end_date is None) or study_date_str is None: | |||
There was a problem hiding this comment.
This is a small thing, but can we disambiguate the names study_start_date, study_end_date, and study_date_str a bit more visually? They all look pretty similar and it makes it a bit difficult to quickly parse this logic. I think it would be helpful to convey with variable names that study_start_date and study_end_date are part of the query, and study_date_str is the attribute of the study being filtered.
I have liked to use the word "candidate" to distinguish this in the past, although I bet there are better ways to do this too. e.g. candidate_study_date instead of study_date_str.
There was a problem hiding this comment.
Renamed in e04c8a1. I used raw_* for the unparsed date strings, which I think is somewhat common for untrusted user input or to indicate an unparsed input.
pacsman/integration_test.py
Outdated
| @@ -163,6 +164,29 @@ def test_local_studies_for_patient(local_client): | |||
| assert ds.StudyInstanceUID | |||
|
|
|||
|
|
|||
| @pytest.mark.integration | |||
| @pytest.mark.local | |||
There was a problem hiding this comment.
Any reason we are using local here instead of remote?
It's a bit confusing, but the way this is currently set up, local tests are not executed in CI. Furthermore, tests using the remote_client fixture (instead of the local_client fixture) do still test the filesystem dicom client in addition to the pynetdicom client. Thus, unless there's a reason why we cannot run this test in CI, I would prefer us to use the remote client here and change the pytest mark to @pytest.mark.remote.
I think that, generally, this integration test distinction between local and remote is a holdover from when we had a local Horos server and a remote third party PACS as a part of testing. At this point, Orthanc hosted in a docker container on CI should supercede both those use cases, so we probably can do away with remote and local entirely. However, that is outside the scope of this PR so no need to make those changes now.
There was a problem hiding this comment.
I think I just got in the coding flow and was going off tests around it. Updated to use remote in 03dfffa.
It's a bit confusing, but the way this is currently set up [...]
I'm glad I'm not the only one that feels this way - definitely took me a few reads of this file on the last PR to understand what was going on 😅 . And definitely agree about fixing up being desirable future work - in fact, I think there are some tests that are actually broken, but never run. Seems like #42, #24, and #22 are all related and could be closed by one PR.
|
@ReeceStevens Just realized I forgot to version bump - figured I would re-request review just to make sure you are good with |
In our
FilesystemDicomClient::studies_for_patientimplementation, there is currently an assumption that if astudy_date_tagfilter is provided, it is of the formatSTART_DATE-END_DATE, like20201014-20201123. However, DICOM actually supports bothSTART_DATE-and-END_DATEformats for range matching. If a user currently tries to use one of those formats, pacsman will throw an exception because it will try to parse a date out of an empty string.This rectifies that, supporting all three date tag formats, and adds an integration test to verify.
The other two clients (
PynetDicomClient,DcmtkDicomClient) should not need to be updated, as they pass the date tag as-is to the underlying system, whereasFilesystemDicomClientwas parsing it to mock the same functionality.