Added fuzzymatching to AttributesUtil.java #91

satheesh09 · 2020-07-02T16:12:58Z

The current version didn't specify configs to enable fuzzymatching attribute that will perform wildcard search on CFIND Requests specifically when partial patient name (first or last name) is supplied as an input parameter. The current version works in such a way that we need to provide the exact patient full name as such it appears in the DICOM tag which would be challenging for any user to provide that as an input to a CFIND query. As per the Google documentation on fuzzymatching (https://cloud.google.com/healthcare/docs/dicom#search_parameters), I added fuzzymatching attribute to negotiate fuzzy semantic patient name attribute matching.

danielbeaudreau

Thanks for the PR!

danielbeaudreau · 2020-07-02T17:13:07Z

util/src/main/java/com/google/cloud/healthcare/imaging/dicomadapter/AttributesUtil.java

@@ -42,6 +42,8 @@

  private static final int INSTANCES_LIMIT = 50000;
  private static final int STUDIES_SERIES_LIMIT = 5000;
+
+  private static final String FUZZY_MATCHING = "true";


Can we make this a boolean flag registered in https://github.com/GoogleCloudPlatform/healthcare-dicom-dicomweb-adapter/blob/master/import/src/main/java/com/google/cloud/healthcare/imaging/dicomadapter/Flags.java so that users could enable or disable it if their backend doesn't support the fuzzy matching parameter?

Yes that's a good idea, I'll make that change and will request your review.

danielbeaudreau · 2020-07-02T17:14:11Z

util/src/main/java/com/google/cloud/healthcare/imaging/dicomadapter/AttributesUtil.java

+
+          if (FUZZY_MATCHING == "true") {
+            qidoPath.append("fuzzymatching=true" + "&");
+          }
+


We can also append fuzzy matching for an instances level retrieval, so I think this if statement could be moved outside of the switch statement, since it would apply for all query retrieve levels

Actually I have tried adding it out of the switch, but I got a build failure for doing fuzzy matching at INSTANCE level. That's why I put it inside STUDY and SERIES level.

What was the build failure? It might be a unit test that needs to be updated?

Yes, it is a unit test.

expected: instances?limit=50000&00080061=MG&
but was : instances?limit=50000&fuzzymatching=true&00080061=MG&
at com.google.cloud.healthcare.imaging.dicomadapter.AttributesUtilTest.testAttributesToQidoPathArray_manyModalitiesSet(AttributesUtilTest.java:105)

Ok, can you please update the tests as well so that the build passes with fuzzy matching enabled for all three retrieval levels?

As per our discussion I have added the fuzzymatching attribute to Flags and referenced it in code. Let me know if everything looks OK after you reviewed my changes.

could you please add unit tests to check what url is have or have not fuzzymatching parameter on different retrieval levels?
also if you don't mind - squash commits into single one

+1, please add a unit test

Looks good overall though. I'm not sure why the contributor license agreement presubmit check isn't running, I'll need to figure that out before we submit this.

added unit test for fuzzy matching.

Added changes to receive CFIND flags from ImportAdapter.java to construct RESTful URL with fuzzymatching

Code alignment

Added changes to send CFIND flags to CFindService.java to construct RESTful URL with fuzzymatching

Added fuzzymatching attribute to perform wildcard search on patient names.

Sending null flag to CFindService upon testing

added null check on flags

Added empty flag for CFIND test case.

Added Unit test for fuzzy matching.

danielbeaudreau

Thanks! I just have a couple more small comments then this is good to submit.

One more thing, can you please verify that you've signed the contributor license agreement with instructions here? https://github.com/GoogleCloudPlatform/healthcare-dicom-dicomweb-adapter/blob/master/CONTRIBUTING.md#contributor-license-agreement

danielbeaudreau · 2020-07-09T17:27:52Z

import/src/test/java/com/google/cloud/healthcare/imaging/dicomadapter/CFindServiceTest.java

+        throw new DicomWebException("test-generated exception", Status.InvalidAttributeValue);
+      }
+    }, Status.InvalidAttributeValue);


Since this isn't testing an error case, I think we should return a dummy response and assert for a success status in both of these tests. E.g:

@OverRide
public JSONArray qidoRs(String path) throws DicomWebException {
JSONArray instances = new JSONArray();
instances.put(TestUtils.dummyQidorsInstance());
return instances;
}
}, Status.Success);

done. I also double checked the contributor license agreement. I can find my Individual Agreements on file that I signed on Jul 02, 2020.

import/src/test/java/com/google/cloud/healthcare/imaging/dicomadapter/CFindServiceTest.java

added assertThat for with and without fuzzymatching

danielbeaudreau · 2020-07-09T20:52:05Z

Thanks, everything looks good, but there's still some merge conflicts, could you please fix those?

satheesh09 · 2020-07-09T21:08:15Z

Conflicts resolved.

danielbeaudreau · 2020-07-09T21:15:07Z

import/src/main/java/com/google/cloud/healthcare/imaging/dicomadapter/ImportAdapter.java

@@ -112,8 +112,8 @@ public static void main(String[] args) throws IOException, GeneralSecurityExcept

    // Handle C-FIND
    IDicomWebClient dicomWebClient =
-        new DicomWebClient(requestFactory, dicomwebAddress, STUDIES);
-    CFindService cFindService = new CFindService(dicomWebClient);
+        new DicomWebClient(requestFactory, flags.dicomwebAddress, STUDIES);


This should be just dicomwebAddress

Update AttributesUtil.java

2c67d6f

satheesh09 requested a review from danielbeaudreau as a code owner July 2, 2020 16:12

Update AttributesUtil.java

2a12abe

danielbeaudreau requested changes Jul 2, 2020

View reviewed changes

satheesh09 added 9 commits July 6, 2020 13:20

Revoked changes as it is not required here.

7e72202

Update CFindService.java

6f40a19

Added changes to receive CFIND flags from ImportAdapter.java to construct RESTful URL with fuzzymatching

Update CFindService.java

095b210

Code alignment

Update ImportAdapter.java

a7816fc

Added changes to send CFIND flags to CFindService.java to construct RESTful URL with fuzzymatching

Update Flags.java

3e9a940

Added fuzzymatching attribute to perform wildcard search on patient names.

Update AttributesUtil.java

9e0a97d

Update CFindServiceTest.java

411efa0

Sending null flag to CFindService upon testing

Update CFindService.java

2e881f8

added null check on flags

Update CFindServiceTest.java

5ccd1a3

Added empty flag for CFIND test case.

satheesh09 requested a review from danielbeaudreau July 6, 2020 18:54

Update CFindServiceTest.java

567129d

Added Unit test for fuzzy matching.

danielbeaudreau requested changes Jul 9, 2020

View reviewed changes

Update CFindServiceTest.java

bc47573

added assertThat for with and without fuzzymatching

satheesh09 requested a review from danielbeaudreau July 9, 2020 19:43

danielbeaudreau approved these changes Jul 9, 2020

View reviewed changes

Merge branch 'master' into master

7acbd07

danielbeaudreau reviewed Jul 9, 2020

View reviewed changes

Update ImportAdapter.java

fb20575

danielbeaudreau approved these changes Jul 9, 2020

View reviewed changes

danielbeaudreau merged commit be7ae71 into GoogleCloudPlatform:master Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added fuzzymatching to AttributesUtil.java #91

Added fuzzymatching to AttributesUtil.java #91

satheesh09 commented Jul 2, 2020

danielbeaudreau left a comment

danielbeaudreau Jul 2, 2020

satheesh09 Jul 2, 2020

danielbeaudreau Jul 2, 2020

satheesh09 Jul 2, 2020

danielbeaudreau Jul 2, 2020

satheesh09 Jul 2, 2020

danielbeaudreau Jul 2, 2020

satheesh09 Jul 6, 2020

pavertomato Jul 6, 2020 •

edited

Loading

danielbeaudreau Jul 7, 2020

danielbeaudreau Jul 7, 2020

satheesh09 Jul 9, 2020 •

edited

Loading

danielbeaudreau left a comment

danielbeaudreau Jul 9, 2020

satheesh09 Jul 9, 2020

danielbeaudreau commented Jul 9, 2020

satheesh09 commented Jul 9, 2020

danielbeaudreau Jul 9, 2020

satheesh09 Jul 9, 2020

Added fuzzymatching to AttributesUtil.java #91

Added fuzzymatching to AttributesUtil.java #91

Conversation

satheesh09 commented Jul 2, 2020

danielbeaudreau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavertomato Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satheesh09 Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

danielbeaudreau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbeaudreau commented Jul 9, 2020

satheesh09 commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavertomato Jul 6, 2020 •

edited

Loading

satheesh09 Jul 9, 2020 •

edited

Loading