1. OpenREM Avatar OpenREM
  2. OpenREM
  3. OpenREM
  4. Issues

Allow complex filtering - NOT, OR

Issue #17 new
Ed McDonagh created an issue

Implement for 1.0 release, with django-filter 2, Python 3, Django 2.2 etc.

Refs #437

@Luuk to bring up to date with current develop and implement as a config option to display. Not sure if it should be in HomePageAdminSettings or in a new SingletonModel. Verbose names to be added to models in an attempt to make selecting the right variable easier!

Update June 2022: Bumped to next release

Comments (117)

  5. Ed McDonagh reporter

    Thanks David. It hadn't occurred to me that the actual database lookups were going to be difficult. Looking at the Q docs, it appears that I hadn't really looked into it very far!

    My initial issue is how to have an interface that allows complex sets of queries in a usable form, both for the end user and the person maintaining the code.

  6. David Platten

    You could have a series of drop-down lists containing field names. Each drop-down list could be separated from the next by a pair of fixed-choice buttons setting "OR" or "AND" between the lists. Where it gets tricky is that you may want the user to be able to use brackets around some of their selections.

  7. Luuk

    Does someone know if their are "plugins" for Django to get a type of filtering on the models like the "advanced" filter options on the Bitbucket issues page or even "more advanced" like pubmed? This last one can also do the "bracket-option". I would think this is a quite common feature needed, but I can't find anything. It might be I'm searching with the wrong terms.

  8. Luuk

    Refs #17 allow complex filtering

    First attempt implementing complex filtering by a query builder form using Django Q objects: - For now only fields in CT models are selectable (DX/RF/Mammo-fields are not added, but could easily) - AND / OR / AND NOT / OR NOT can be chosen to connect constraints on fields - Brackets can be used (first and last textbox, this might be unclear for the user) - For now the search is completely modality independent, although results use CT template and only CT models are loaded - For now page is only reachable by manually entering http://server:port/openrem/advanced_search

    Things that need to be done (at least): - Check for validity of user input * only decimals in decimal fields, dates in date fields, etc.. * Correct placing of brackets - Results for queries without brackets are almost certainly correct, but with brackets need to be tested thoroughly - If query is sent, the results page should also contain the query as built by the user (textboxes should be filled). So the user can adapt the query easily - Think about available fields...There are a lot. Do we need to sort or limit them? - Verbose name is used for displaying field names, but we may need to add these to the model for a number of fields for clarity - Should it be possible to select "NOT" for the first field search? - Pressing <Enter> while "add line" is selected will add extra line, but selected "add line" icon remains visible and next line added is empty - Write test script

    → <<cset 638fb3e0f115>>

  9. Luuk

    I just committed a first attempt for making complex filtering possible (advanced search). It isn't finished by far, but I'm really curious what you (@edmcdonagh, @dplatten, @tcdewit) think about the principal and possibilities. See commit 638fb3e, for some additional remarks

  14. Luuk

    Refs #17 allow complex filtering

    First attempt implementing complex filtering by a query builder form using Django Q objects: - For now only fields in CT models are selectable (DX/RF/Mammo-fields are not added, but could easily) - AND / OR / AND NOT / OR NOT can be chosen to connect constraints on fields - Brackets can be used (first and last textbox, this might be unclear for the user) - For now the search is completely modality independent, although results use CT template and only CT models are loaded - For now page is only reachable by manually entering http://server:port/openrem/advanced_search

    Things that need to be done (at least): - Check for validity of user input * only decimals in decimal fields, dates in date fields, etc.. * Correct placing of brackets - Results for queries without brackets are almost certainly correct, but with brackets need to be tested thoroughly - If query is sent, the results page should also contain the query as built by the user (textboxes should be filled). So the user can adapt the query easily - Think about available fields...There are a lot. Do we need to sort or limit them? - Verbose name is used for displaying field names, but we may need to add these to the model for a number of fields for clarity - Should it be possible to select "NOT" for the first field search? - Pressing <Enter> while "add line" is selected will add extra line, but selected "add line" icon remains visible and next line added is empty - Write test script

    → <<cset faabb7ec3cdc>>

  19. Luuk

    Refs #17 Advanced Filtering

    • Added advanced filtering option to CT filtered page as example (can be quite easily implemented for other modalities)
    • Needed to rewrite ctfiltered template as "form" has to extend multiple blocks
    • Made it possible to swich from exam filter to advanced filter and vice versa
    • Improved advanced filtering with possibility having NOT in first line
    • Updated logic to never negate root node (but always add a NOT node)
    • Updated logic to respect logic for user specific "pid"
    • Updated logic to be able to insert filter on modality independent of user input (depending on modality viewed)
    • Resolved PEP8 issues in mod_filters

    → <<cset 406a6f51acae>>

  23. Luuk

    I just uploaded an advanced filtering option in the CT filtered page. I'm rather happy with it (of course there are some "issues'). It can be quite easily build in the DX and RF filtered pages also.

    Some "highlights":

    • The idea of the advanced filtering is based on the Pubmed advanced search
    • Default behaviour of the CT filtered is unchanged
    • It is possible to switch from the default "exam filter" to the advanced filtering within the CT filtered page and vice versa
    • Currently a lot of filter parameters can be selected (should they be sorted? reduced?).
    • You can use brackets and AND/OR/NOT operators in the search options (next to the comparison operators depending on the field type).

    Some Remarks:

    • Currently the plots don't respect the advanced filter
      • I don't understand how data flows to the plots, but it seems dependent on the specific "exam filter" fields
      • It would be great if you could pass a filter- or querset-object
    • I tested it with a limited number of datasets and it seems to do the right thing, but I'm not sure if it works in all cases
    • I think it is unclear that the textbox just before the filterfield and the last textbox in each line are meant to place brackets "(" or ")"
      • Any ideas how to improve this?
    • Checking that "number-fields" only contain a number in the filter value is not implemented (yet, same holds through for dates)
    • In the end it would be great if you would be able to save (and load) queries
      • Then it would also be nice if date-fields could be relative ("from on year ago till now")
      • For now you can just copy-paste URLs

    @edmcdonagh, @dplatten what do you think? Any suggestions to improve this filtering?

  25. David Platten

    This would be useful when wanting to filter to just “CT head” requests, but exclude “CT head with contrast” requests. I would like to be able to do this for the PHE CT dose export (#737); it is likely to be useful for the PHE radiographic and fluoroscopy survey too (#750)

  26. Ed McDonagh reporter

    Hmm. I don’t think we are going to be able to do this in the current beta/release. For version 1.0, I am intending to start again with django-filters (current release version), and make sure we learn from all the work that @LuukO has done here to implement AND/OR/NOT in the standard filter view (including charts). If you have any ideas about that, I’d be please to hear them.

    For your current situation - is there anything in the combination of study description, requested procedure, procedure etc that can narrow down what you need instead?

  28. David Platten

    I think that incorporating the logic in the standard filter view is the way to go.

    For my current situation I’m unable to filter to avoid a little “pollution” from “…with contrast” studies.

  29. Ed McDonagh reporter

    I’ve been trying to work out how to move this forward @Luuk , @David Platten , and I’ve not got very far!

    I’m assuming that after the changes for Python 3, Django 2.2 and most importantly django-filters 2.2, we can’t easily slot in Luuk’s code from before? I also can’t exactly remember how it was implemented in the user interface?

    One of the things I have been looking at is whether there was a way of using a library of some sort to parse the string so that we don’t have to maintain the code to find all the brackets and operators etc. The closest I have come to is Luqum which can parse the query string and manipulate it, primarily for feeding into Elasticsearch but it might be useful. For instance if we had a querystring of ((model:"*flash" AND study:"Thorax^TAP_IV") OR (model:"*edge” AND study:”Thorax^TAP_120kV”)) AND NOT station:”CT Two” we could use Luqum to find all the operators and groups and wildcards and phrases and words and child nodes etc, and we can swap the name model for generalequipmentmoduleattr__manufacturer_model_nameetc. But I don’t know how to then feed the result into django-filter or filtering using standard Django filters.

    I also don’t know how we should present this to the user. In my mind, I’d like to have some sort of query builder - either a single box to choose a filter, then one pops up to choose a filter type then a value one pops up, and you can add groups and further filters; or a text input box with buttons to add appropriate filter and operator text. What I’d like to move away from is a list of 30 or more boxes down the right hand side, which really restricts how many filters you can offer because it becomes unusable in how much scrolling you need to do!

    What do you think? Am I being too radical? I’m keen not to delay version 1 release for too long, so we could go conservative in this release where filtering is concerned, but it would be a real improvement if we could crack it!

  30. Luuk

    Hi @Ed McDonagh , @David Platten ,

    I had a more or less working version in which the default filtering (as we have it now) was shown first. In the filter header you could click on “complex filtering” and then the side bar filtering disappeared and above the modality rows a method to make “complex” filter rules appeared. Here also an option was available to return to the default filtering.

    Issues, in my opinion, were:

    • The number of possible parameters is large, how to maintain overview for the user?

      • Make subsets with parameters belonging to each other?

    • You need somehow to set round brackets, this was a bit rudimentary in my implementation, but I didn’t know how to do this nicely.

    I can have a look if I can make this version available. But it might take till after ECR (but maybe I find some time before). It was of course also made for Python 2.7/Django 1.8.

    I also looked (back then) to other libraries, but I couldn’t find anything suitable.

    ‌

  31. Ed McDonagh reporter

    Hi @Luuk Have you had a look at this again?

    I’m thinking that it might need unfortunately to be bumped again to a future release.

    What do you think?

    Hope you are well.

    Ed

  32. Luuk

    Hi @ed,

    I’ll dig this up and see if I can make it work using the current version.
    Then we need to decide if it is usable.

    Let you know the progress.

    Luuk

  34. Luuk

    Refs Issue #17

    First attempt to paste complex filtering code in Python3/Django2 environment Not functional at all, looks crappy

    [skip ci] as not functional at all.

    → <<cset 56c7ced4395a>>

  35. Luuk

    Refs #17 - Advanced filtering look better now - Filtering itself (default and advanced) seems to work - Added icons for adding / deleting advanced search lines

    Todo: - Improve layout - Find nicer way to set brackets "(" and ")" - After applying advanced filter remain in advanced filter mode - More testing

    [skip ci] (not ready)

    → <<cset 24ac56422738>>

  36. Luuk

    Refs #17 (Complex filtering)

    • Filtering working now (it seems)

    • Layout (rather) Ok

    • Replaced 'var' with 'let' in javascript (as per 'new' ECMAscript specifications)

    Didn't look at the graphs as these are reworked now.

    → <<cset 36783f276830>>

  37. Luuk

    @Ed McDonagh @David Platten @wens and others, I updated the “complex filtering” code I had before. It is possible to use it now in this branch(https://bitbucket.org/openrem/openrem/branch/Issue17ComplexFilteringPython3) for CT.

    You can switch from “default” to “advanced” filtering and vice-versa by using links in the filter-headers on the ct detail pages. If the advanced filter is shown, the default isn’t (and the other way around)

    The advanced filter is positioned above the filtered results and looks like this ('verzenden' is Dutch, but I suppose it will show ‘Submit query’ using English language settings):

    Graphs currently don’t work with the filter as the current graphs look at the filter itself (instead of using the queryset). But as they are reworked now, I didn’t look into this.

    I think it works reasonably at the moment, I’m not so happy with the way to set brackets, but don’t know another way to do it. I don’t want users to type/edit the “search string” (in the grey bar) for this as that could create invalid brackets (in position and/or number).

    Another issue is the number of parameters, they are enormous. Should we add them all or filter? And some model field names are used multiple times (in other models, e.g. ‘comment’). Should we rename them, so we know which ones they are (or add the model name (a user-friendly model name) to (all) parameters)?

    Any comments/ideas are welcome!

  38. Ed McDonagh reporter

    Very nice, thank you for this @Luuk - I’ve thrown the branch onto https://testing.openrem.org to take a look at it (@David Platten don’t be surprised when your charts aren’t there! Feel free to change it back to your branch as you need to).

    For the parameter list, they are currently not CT specific, and we have duplicates, some of which presumably wouldn’t work as we expect them to. I haven’t looked at the code yet to see how they are derived, but yes, we’ll need to curate them somehow.

    For the brackets, I wonder if you can have a click button to add/remove brackets at the start and end of each clause, instead of free text (albeit restricted to the ( ) characters!).

    I might be wrong, but it also feels slower than the classic filtering. I need to get my head back into database query performance for both this and the study acquisition issue I’m having!

    The button just says ‘Submit’ by the way!

  39. Luuk

    I think the parameter list is CT specific (of course including some “general” model fields). The fields from the following models are added:

    GeneralStudyModuleAttr
    PatientModuleAttr
    PatientStudyModuleAttr
    GeneralEquipmentModuleAttr
    UniqueEquipmentNames
    CtRadiationDose
    CtAccumulatedDoseData
    CtIrradiationEventData
    CtXRaySourceParameters
    ScanningLength
    SizeSpecificDoseEstimation
    CtDoseCheckDetails

    In principle all models can be added as long as you can define a relationship starting from the GeneralStudyModuleAttr.

    For the brackets, I’ll try to add “add”/”remove” “(“ and “)” buttons, that might be better indeed. Now you can’t type other characters than “(“ and “)“, but you would be able to paste it.

    I don’t know if the query is slower (it might be), but anyway it needs to do some pre-processing before it can query at all (changing the query-string into a real query that is understandable by the Django-framework).

    ‌

  42. Ed McDonagh reporter

    Hi @Luuk I’ve just had a look at the merged issue 17 branch and the pipeline test errors. I don’t think the merge of ctfiltered.html went very well - most of the content is missing, so currently only the number of studies is displayed!

    If you need any help, please ask 🙂

  46. Luuk

    Refs issue #17 complexing filtering

    • Working filtering for CT.
    • HTML files for modalities are finished
    • views.py has to be adapted for dx, mg and rf.
    • In advanced_search_functions "get_advanced_search_options" must be added for dx, mg and rf
    • In models.py "verbose_name" has to be added to make name of filter parameter more clear

    [skip ci]

    → <<cset 4f0ee23e207e>>

  48. Luuk

    Refs issue #17

    Fixed duplicate lines in views.py and views_chart_ct.py Added verbose_names in models.py (where needed)

    Next step: Make it possible to show / hide this option for the user. [skip ci]

    → <<cset 9fb61f08ffcc>>

  49. Luuk

    Refs issue #17

    Added user-option to enable complex filtering Replaced manual placing of brackets by +/- buttons.

    TODO: - Add option to other modalities - Solve bug: if closing brackets are before opening brackets, javascript thinks it is fine.

    → <<cset 0682178a6561>>

  51. Luuk

    Refs issue #17

    • Generalized adding json search options to prevent copying code

    • Added option to DX module (todo: add verbose_names to models, check search options, test)

    • Found "bug", if query is clicked in paginator of graph options, advanced search is changed to default filtering.

    → <<cset 025121adbcaa>>

  52. Ed McDonagh reporter

    I feel really bad about this @Luuk - with all the work that you have put in over the years! But the codebase has got too far ahead again before we go this nailed down, so I am going to have to push the target release out again so we can get it working with the current codebase and get it finished off. Sorry!

  55. Kevin Schärer

    Refs #17

    Second draft for advanced filtering One can add, edit and remove filters in the NM front-end. The back-end can process the request and return the entries which have matched the filters

    → <<cset 419468e1b2a2>>

  57. Kevin Schärer

    Refs #17

    Second draft for advanced filtering One can add, edit and remove filters in the NM front-end. The back-end can process the request and return the entries which have matched the filters

    → <<cset e77e691e1fec>>

  59. Kevin Schärer

    Hi @Ed McDonagh

    The approach I am currently taking is to completely replace the current exam filter with the new advanced filter. This way, it is still possible to use a simple filter (by just adding a single filter). However, there is now the ability to attach and logically link additional filters to have better control over which study entries you want to see.

    This is currently implemented by using a dictionary (which is then converted to a JSON string) that contains all the information about the filters. Then the backend builds a Q object from the given JSON string. I still use django-filters because this way we can still define fields that should not have a different lookup type (like get for dates) and a specific format; so for the date field, a selectable calendar is still displayed in the front-end.

    Currently, nested groups are not implemented yet, as I wanted to get back to you to see if my approach makes sense after all.

    To give you a better idea, I'm attaching a recent screenshot.

    ‌

  61. Ed McDonagh reporter

    Sorry for not getting back to you earlier @Kevin Schärer - I think it sounds like the right approach though. I’ll check it out this evening to take a look at what it looks like to use. What do you think @David Platten ?

  66. Kevin Schärer

    Refs #17

    Introduces a filter-library to store the built pattern Currently one can only load an existing filter I'll add support for adding and removing later

    → <<cset de540b80c1cc>>

  69. David Platten

    @KevinSchärer and @Ed McDonagh I think that the new filtering is good, and am happy for it to be implemented for the other modalities.

    I would like a way of adding “does not contain” and “is not” to the filtering of fields.

  74. Ed McDonagh reporter

    @Kevin Schärer - can all the text fields have options on them, and by default be contains?

  75. Ed McDonagh reporter

    A feature I discussed with David earlier would be whether you could have a library for your user, with admin accounts being able to promote queries to a system wide library?

  77. Kevin Schärer

    @Ed McDonagh - Since yesterday (d293a50 - I didn't mention it in the commit itself…) one can configure which fields should have options on them.

    I will set the default to icontains together with a user specific and system wide library, later this day.

    ‌

  79. Kevin Schärer

    Refs #17

    • Set icontains as default lookup type
    • There is now a user specific and system-wide library, where only admins can manage the shared (system-wide) patterns

    → <<cset d2a7bb7b8d28>>

  80. Ed McDonagh reporter

    Hi @Kevin Schärer - in the last commit you added a user field that generates this output when doing makemigrations: 

    (virtualenv) deploy@localhost:~/sites/testing.openrem.org/source$ python openrem/manage.py makemigrations remapp
You are trying to add a non-nullable field 'user' to filterlibrary without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit, and let me add a default in models.py
Select an option:

    ‌

  83. Ed McDonagh reporter

    Low priority fix - the name field accepts lots of characters, but then does nothing if you use more than 32 characters. I think a combination of making the field longer and restricting the input to match would be good :)

  96. Kevin Schärer

    Hi @Ed McDonagh & @David Platten

    I've applied the new filter system to all modalities and created a user-specific and system-wide library. I've also adapted the relevant test cases to the new filter.

    The question now is what changes need to be made to make the filter as usable as possible. One thing that came to mind was to allow a filter to be moved into or out of nested groups without having to create a new filter and delete the old one. What else do you think could be done?

    Thanks in advance!

    ‌

  99. Ed McDonagh reporter

    Thanks for this @Kevin Schärer . I don’t think I can see that a filter is not, like the saved filter: Contains chest and not abdomen.

    Can you move the export box to the main column and use the whole width for everything?

    Can you also pull the develop branch into your branch too please.

    Thanks again!

  103. Kevin Schärer

    Hi @Ed McDonagh

    Retain pattern means to make a shared pattern private again. The wording is not really sophisticated, I suppose… 😀

  104. Ed McDonagh reporter

    I see! Some suggestions:

    • Use Save current filter, Your filters, Filters shared with all users instead of patterns? Or maybe Save current filter pattern etc?
    • Label the Library box Filter library or Filter pattern library?
    • Instead of Share pattern, maybe Make available for all users?
    • Instead of Retain pattern, maybe Remove from all users?
    • Wherever we are introducing strings, can you please try to make them part of the translation file - see the docs for making template strings translatable and elsewhere on the same page for any python strings.
    • I think it would be better if the Filter box started collapsed. Looking at your code, it looks like it should be? But I haven’t played with Bootstrap for a long time so I might be missing something. For me it is always open on first load, with the Add filter button staring at me.

    Thanks @Kevin Schärer

    ‌

  106. Kevin Schärer

    Hi @Ed McDonagh

    I’ve implemented your suggestions - including the new wording and string-translation

    The filter box was not collapsed initially, as the in class was still attached to it...

    Next, I will add some unit tests for the json_to_query function, which translated the given filter (as JSON) to a QuerySetobject. Although the function gets indirectly tested by some integration tests, I think it makes it easier to spot a potential issue after future changes. Any thoughts?

  107. Ed McDonagh reporter

    I’m very keen on good tests, so please go ahead.

    We need to be careful to check that series level filters return the right number of studies - if a filter matches more than one series of a study, it should still count as one study for example.

    Any thoughts, @David Platten ?

    We also need to make sure the charts and exports are working and returning the right results.

    ‌

  108. Kevin Schärer

    When applying this filter (on the old filtering) it should show no studies at all, since no acquisition DAP falls inside the specified range, right? The same happens when using the new filtering.

    I’ve discovered that when one uses the NumericRangeFilter instead of a min and max field, as it is already done for the Mammography modality, the right result (in my opinion) is shown. This is probably an issue with the generated SQL query, I assume.

    Should we change all those fields into range fields @Ed McDonagh ?

  109. Ed McDonagh reporter

    I’m not sure what is going on there Kevin - the study that is shown has an exposure that is 0.57 and another that is 0.82, so I don’t know how a filter of 0.7 min, 0.7 max is matching it.

    A disadvantage of the mammo breast thickness range filter (is that the one you are referring to?) is that you need to put both ends of the range in for it to work. Not the end of the world though if it gets the right result!

  110. Kevin Schärer

    Yes, that's what I meant. The django-filter module has another range class called RangeFilter. This allows you to have a range or just a minimum or maximum. So, it would be a suitable replacement for the current setup. (@Ed McDonagh )

  116. Kevin Schärer

    Hi @Ed McDonagh

    All min & max fields have now been replaced by range fields (using RangeFilter as specified earlier), where one does not lose any filter functionality compared to the legacy setup.

    I’ve also added some tests to check the introduced backend and frontend changes accordingly.

    I think the current state of the new filter system is more or less where I wanted it to be.
    Regarding the tests for the graphs and exports: I think these are already covered with the old tests, since the input is the same as for the filter pages, whose content is after all tested by my new tests. Or do I miss something?

    Thanks.

  118. Log in to comment
Assignee
Type
enhancement
Priority
major
Status
new
Component
Interface
Milestone
1.1.0
Votes
0
Watchers
2
Jira Software: the preferred issue tracker for Bitbucket. Join the team!