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

Search in dicom metadata #2450

Merged
merged 8 commits into from Dec 6, 2017

Conversation

Projects
None yet
6 participants
@Pierre-Assemat
Copy link

commented Oct 30, 2017

The dicom_viewer plugin add a new search modes into the search mode registry :

  • Substring search in both keys and values

Fixes: #2313

@Pierre-Assemat Pierre-Assemat self-assigned this Oct 30, 2017

@Pierre-Assemat Pierre-Assemat force-pushed the search-in-dicom-meta branch 4 times, most recently from 55ec0fb to 9d3bd3c Oct 30, 2017

@Pierre-Assemat Pierre-Assemat requested a review from brianhelba Nov 1, 2017

@jcfr

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

@fedorov Once integrated, this will add DICOM metadata search.

Cc: @mgrauer

@Pierre-Assemat Pierre-Assemat force-pushed the search-in-dicom-meta branch 2 times, most recently from 13e1662 to d9ca96b Nov 6, 2017

@Pierre-Assemat Pierre-Assemat force-pushed the search-in-dicom-meta branch 4 times, most recently from 2086e8f to ded321c Nov 15, 2017

},

getListMode: function () {
const modeList = [];

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

There is an _.keys method in Underscore that does this.

@@ -152,7 +153,8 @@ var SearchFieldWidget = View.extend({
content: _.bind(function () {
return SearchModeSelectTemplate({
modes: this.modes,
currentMode: this.currentMode
currentMode: this.currentMode,
descriptionModes: SearchFieldWidget.getModeDescription

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

Let's pass this in as getModeDescription instead of descriptionModes.

return SearchFieldWidget._allowedSearchMode[mode].modeDescription;
},

getHelpDescription: function (mode) {

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

Can we change this globally to getModeHelp?

},

getHelpDescription: function (mode) {
return SearchFieldWidget._allowedSearchMode[mode].helpDescription;

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

Let's call the property .help internally.

},

getModeDescription: function (mode) {
return SearchFieldWidget._allowedSearchMode[mode].modeDescription;

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

Let's call this .description internally.

def load(info):
Item().exposeFields(level=AccessType.READ, fields={'dicom'})
events.bind('data.process', 'dicom_viewer', _uploadHandler)

# Add the DICOM search mode only once
if search.getSearchModeHandler('dicom') is None:

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

You can safely assume that load will only be called once, so you don't need the conditional.

Provide a substring search on both keys and values.
"""
if types != ['item']:
raise TypeError('The dicom search is only able to search in Item.')

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

This should be a RestException, to ensure it raises a 400 Client error. A TypeError will end up raising a 500 Server error.

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

Also, please ensure that you import RestException from girder.exceptions, as we recently changed it.


# Filter the result
result = {
'item': [Item().filter(d, user) for d in cursor]

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

We need to do more post-processing before returning this list. We need to:

  1. Filter the items to only those that user can access at level
  2. Offset by offset items
  3. Limit to limit items
  4. Filter the fields of each item to those that user can access at level

The first 3 can be done by Item().filterResultsByPermission, but you need to call it explicitly. The 4th is already being done by you in the call to Item().filter.

mongoQuery = {'dicom': {'$exists': True}, '$where': jsQuery}

# Sort the documents inside MongoDB
cursor = Item().find(mongoQuery)

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

I think you can inline the mongoQuery variable here.

obj.dicom.meta[key].toString().toLowerCase()
.indexOf('%(value)s'.toLowerCase()) !== -1;
})
""" % {'key': query, 'value': query}

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 5, 2017

Member

Right now, there's no need for the return, since $where can accept a simple JavaScript expression.

However, I think you should wrap this whole jsQuery into a JS function, so we can define some intermediate variables for performance. Specifically, I think you should define variables for at least:

  • '%(key)s'.toLowerCase()
  • '%(value)s'.toLowerCase()
  • obj.dicom.meta
@brianhelba

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

Looks really good so far. Comments inline, mostly changes to naming and user-facing English strings.

Pierre Assemat added some commits Oct 30, 2017

Pierre Assemat
Server: dicom_viewer: Substring search in dicom metadata
The DICOM search is able to provide only Item search, it will raise
an exception if using the wrong type.
Pierre Assemat
Client: Core: Add a search mode Registry with internal API
Allow external plugin to add new search mode on the client side.
This mode registry contain :
- mode description
- help description
- allowed types
Pierre Assemat
Test: Add new DICOM search test & fix tests
- fix style of tests
- import plugin module directly in function

@Pierre-Assemat Pierre-Assemat force-pushed the search-in-dicom-meta branch 3 times, most recently from f1da1c0 to 1a1fa9d Dec 5, 2017

# We will need to search into object value as improvment.
jsQuery = """
function() {
const key = '%(key)s'.toLowerCase();

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 6, 2017

Member

const is ES6 syntax. For compatibility with older versions of MongoDB, we should only use ES5 syntax, so this should be var.

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 6, 2017

Member

Also, can you name this queryKey and queryValue? In addition to being clearer, this will fix a bug in the current implementation.

function() {
const key = '%(key)s'.toLowerCase();
const value = '%(value)s'.toLowerCase();
const meta = obj.dicom.meta;

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 6, 2017

Member

Can you name this dicomMeta?

# Sort the documents inside MongoDB
cursor = Item().find({'dicom': {'$exists': True}, '$where': jsQuery})
# Filter the result
result = Item().filterResultsByPermission(cursor, user, level, limit, offset)

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 6, 2017

Member

You can inline this filterResultsByPermission into the list comprehension below.

This comment has been minimized.

Copy link
@Pierre-Assemat

Pierre-Assemat Dec 6, 2017

Author

What do you mean by inline this ?

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 6, 2017

Member

Combine this line and the next.

This comment has been minimized.

Copy link
@Pierre-Assemat

Pierre-Assemat Dec 6, 2017

Author

But filterResultsByPermission take a cursor, and filter take only one element, how can I compact them into one line ?
Something like:

result = {
        'item': Item().filterResultsByPermission([Item().filter(d, user) for d in result], user, level ...)
}

is ok ?

This comment has been minimized.

Copy link
@zachmullen

zachmullen Dec 6, 2017

Member

'item': [Item().filter(i, user) for i in Item().filterResultsByPermission(...)]

This comment has been minimized.

Copy link
@Pierre-Assemat

Pierre-Assemat Dec 6, 2017

Author

In order to fit in the 100 characters limitation what is the best :

result = {
        'item': [Item().filter(d, user) 
            for d in Item().filterResultsByPermission(cursor, user, level, limit, offset)]
    }

or

    result = {
        'item': [Item().filter(d, user) for d in Item().filterResultsByPermission(
            cursor, user, level, limit, offset)]
    }

or again

    result = {
        'item': [
            Item().filter(d, user)
            for d in Item().filterResultsByPermission(cursor, user, level, limit, offset)
        ]
    }

This comment has been minimized.

Copy link
@zachmullen

zachmullen Dec 6, 2017

Member

I personally like the third one, but the second one is OK too.

This comment has been minimized.

Copy link
@Pierre-Assemat

Pierre-Assemat Dec 6, 2017

Author

Me too, but is this easily readable ?

This comment has been minimized.

Copy link
@zachmullen

zachmullen Dec 6, 2017

Member

I think it is, it's a fairly common way to break up a list comprehension. If you don't think it's readable enough, you could always make a variable called cursor and use that in the comprehension instead of inlining the filterResultsByPermission call.

This comment has been minimized.

Copy link
@Pierre-Assemat

Pierre-Assemat Dec 6, 2017

Author

That's good to me, thank you Zach!

_allowedSearchMode: {},

addMode: function (mode, types, description, help) {
if (!SearchFieldWidget._allowedSearchMode[mode]) {

This comment has been minimized.

Copy link
@brianhelba

brianhelba Dec 6, 2017

Member

This is minor, but in my opinion, it's probably clearer style to check whether the mode already exists using _.has, and if so, throw the error within the body of the if. Then, do the normal work of registering the mode in the body of the function (not inside an if or else).

I think this is preferable because it tests any preconditions at the start of the function call, then moves into the function logic with the assumption that they're true. This allows all the logic to be at the top (non-indented) level of the function, and keeps the error handling (throw an exception) close to the error check (the condition).

Pierre Assemat
Client: Names changes for consistency
This changes also the UI descriptions and helps
using different search mode.

@Pierre-Assemat Pierre-Assemat force-pushed the search-in-dicom-meta branch from 1a1fa9d to ee0066c Dec 6, 2017

@Pierre-Assemat Pierre-Assemat force-pushed the search-in-dicom-meta branch from ee0066c to 3042346 Dec 6, 2017

@brianhelba
Copy link
Member

left a comment

LGTM!

@Pierre-Assemat Pierre-Assemat merged commit 25f87e5 into master Dec 6, 2017

2 of 3 checks passed

codecov/patch 80.95% of diff hit (target 90.47%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 90.41% (-0.07%) compared to bbdad8b
Details

@Pierre-Assemat Pierre-Assemat deleted the search-in-dicom-meta branch Dec 6, 2017

@mgrauer

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Great stuff, way to go guys.

types: JSON.stringify(_.intersection(
this.types,
SearchFieldWidget.getModeTypes(this.currentMode))
)

This comment has been minimized.

Copy link
@msmolens

msmolens Mar 2, 2018

Contributor

Performing an intersection here breaks search results when SearchFieldWidget is initialized with a custom type, because types will be empty.

@mgrauer mgrauer referenced this pull request Mar 21, 2018

Merged

v2.5.0 #2673

mgrauer added a commit that referenced this pull request Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.