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.

Sign up for GitHub

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

Added fuzzymatching to AttributesUtil.java #91

Merged
merged 15 commits into from
Jul 9, 2020
Merged

Added fuzzymatching to AttributesUtil.java #91

Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2c67d6f
Update AttributesUtil.java
satheesh09 Jul 2, 2020
2a12abe
Update AttributesUtil.java
satheesh09 Jul 2, 2020
7e72202
Revoked changes as it is not required here.
satheesh09 Jul 6, 2020
6f40a19
Update CFindService.java
satheesh09 Jul 6, 2020
095b210
Update CFindService.java
satheesh09 Jul 6, 2020
a7816fc
Update ImportAdapter.java
satheesh09 Jul 6, 2020
3e9a940
Update Flags.java
satheesh09 Jul 6, 2020
9e0a97d
Update AttributesUtil.java
satheesh09 Jul 6, 2020
411efa0
Update CFindServiceTest.java
satheesh09 Jul 6, 2020
2e881f8
Update CFindService.java
satheesh09 Jul 6, 2020
5ccd1a3
Update CFindServiceTest.java
satheesh09 Jul 6, 2020
567129d
Update CFindServiceTest.java
satheesh09 Jul 9, 2020
bc47573
Update CFindServiceTest.java
satheesh09 Jul 9, 2020
7acbd07
Merge branch 'master' into master
satheesh09 Jul 9, 2020
fb20575
Update ImportAdapter.java
satheesh09 Jul 9, 2020
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
12 changes: 12 additions & 0 deletions util/src/main/java/com/google/cloud/healthcare/imaging/dicomadapter/AttributesUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ public class AttributesUtil {

private static final int INSTANCES_LIMIT = 50000;
private static final int STUDIES_SERIES_LIMIT = 5000;

private static final String FUZZY_MATCHING = "true";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


public static String getTagValue(JSONObject json, String tag) throws JSONException {
JSONObject jsonTag = json.getJSONObject(tag);
Expand Down Expand Up @@ -118,9 +120,19 @@ public static String attributesToQidoPath(Attributes attrs, String... includeFie
switch (attrs.getString(Tag.QueryRetrieveLevel)) {
case "STUDY":
qidoPath.append("studies?limit=" + STUDIES_SERIES_LIMIT + "&");

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@pavertomato pavertomato Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, please add a unit test

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@satheesh09 satheesh09 Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

added unit test for fuzzy matching.

break;
case "SERIES":
qidoPath.append("series?limit=" + STUDIES_SERIES_LIMIT + "&");

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

break;
case "IMAGE":
qidoPath.append("instances?limit=" + INSTANCES_LIMIT + "&");
Expand Down