- changed component to Interface
Allow complex filtering - NOT, OR
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)
-
reporter -
reporter - changed milestone to Future
-
reporter New django-filter version 0.8 has exclude as an option.
-
Hi Ed. You could use the "Q" filtering that I made use of for "CR" or "DX" types to construct logical "OR"s. See https://docs.djangoproject.com/en/dev/topics/db/queries/
-
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.
-
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.
-
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.
-
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>>
-
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
-
reporter I've thrown it on to http://testing.openrem.org/openrem/advanced_search and it looks interesting. I didn't manage to get any results with my initial attempt though. Is display name not in the list?
-
-
I see I didn't add that model. Now I did in the issue17AdvancedFiltering branch But I tried Manufacturer is Siemens AND Total Number of Irradiation Events is 2 and that worked fine for me.
-
I've had a play and it seems to work fine too. I like the "AND NOT" option. Ed's query above, but for Siemens studies that are not 2 2 events:
-
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>>
-
-
Refs #17 First Changes
→ <<cset 76eabffaca45>>
-
Refs #17
Attempt to include advanced filter in "filtered" page for CT.
(a lot has to be done)
→ <<cset d4af231b8bfb>>
-
Refs #17
→ <<cset b7a8fb6a934f>>
-
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>>
-
-
-
Refs Issue #17
- Removed (now) unneccessary file advancedsearch.html
- Solved other merge errors (last now)
- Added Docstring to AdvancedSearchFilter
→ <<cset 443b72c5acab>>
-
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?
-
-
-
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?
-
reporter - edited description
- changed milestone to 1.0.0
Updated release milestone, description.
-
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.
-
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 namemodel
forgeneralequipmentmoduleattr__manufacturer_model_name
etc. 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!
-
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.
-
-
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
-
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
-
reporter Thanks Luuk, really appreciate that
-
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>>
-
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>>
-
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>>
-
-
@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!
-
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!
-
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
CtDoseCheckDetailsIn 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).
-
reporter -
assigned issue to
- edited description
-
assigned issue to
-
reporter - edited description
-
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
-
After the merge I can't even get the manage.py to run. So I need to check other stuff before....😉
-
Working again…And I see, it broke down the CT detail page.
-
Refs #17 Overview is back again. Graphs are not shown, Number of items per page is not respected
[skip ci]
→ <<cset 18c3af71f457>>
-
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>>
-
Refs issue #17
- Now charts are also working, but I'm not happy with it as I had to duplicate code (25 lines)
- Made import relative.
[skip ci]
→ <<cset 3f14337ce85b>>
-
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>>
-
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>>
-
Refs issue #17
Solved bug concerning the brackets First attempt to add models for 2d plane imaging (completely untested)
[skip ci]
→ <<cset bc5acacdbbf6>>
-
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>>
-
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!
-
reporter - changed milestone to 1.1.0
- edited description
-
Refs #17
First draft for advanced filtering. For the meantime I will do the changes only on the NM modality.
→ <<cset c06e6f6d78c9>>
-
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>>
-
Refs #17
First draft for advanced filtering. For the meantime I will do the changes only on the NM modality.
→ <<cset 629fb5e4c778>>
-
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>>
-
-
assigned issue to
-
assigned issue to
-
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 usedjango-filters
because this way we can still define fields that should not have a different lookup type (likeget
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.
-
-
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 ?
-
-
reporter Hi @Kevin Schärer , @David Platten - I’ve pushed this branch to the testing server: https://testing.openrem.org/openrem/nm/ (demo/demo).
I think it is the right approach. David, I’d value your opinion too.
Thanks Kevin.
-
Refs #17
Removing leftovers from pattern object when new filters are discarded rather than saved Remove exam filter from NM filter page
→ <<cset c07f641b7063>>
-
Refs #17
Fix issue, where the current filter gets deleted when the date-picker modal is closed (see c07f641)
→ <<cset cd3c16d7c7da>>
-
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>>
-
-
Refs #17
- Add ability to delete a specified pattern
- Adjusted the back-end handling
→ <<cset 3d8dc950162a>>
-
@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.
-
-
-
-
-
reporter @Kevin Schärer - can all the text fields have options on them, and by default be contains?
-
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?
-
reporter (I mean
icontains
of course) -
@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.
-
reporter Thank you!
-
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>>
- Set
-
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:
-
-
@Ed McDonagh - I’ve changed the field to allow null entries
-
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 :)
-
Refs #17
- Increases
name
field size and restricts the related input field max-size - deletes drop-down field if there is no option left
→ <<cset 24b7ca1f650a>>
- Increases
-
-
-
-
-
Refs #17
- Adjusts tests for filtering tasks
- Fixes
KeyError
exception inmod_filters.py
→ <<cset b54acf7af64d>>
-
-
-
-
-
-
Refs #17
Fixes a minor issue where the date picker was not displayed anymore
→ <<cset 1ecbf9ebfdf7>>
-
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!
-
-
-
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!
-
-
-
reporter Thanks @Kevin Schärer - what does “Retain pattern” mean?
-
Hi @Ed McDonagh
Retain pattern
means to make a shared pattern private again. The wording is not really sophisticated, I suppose… -
reporter I see! Some suggestions:
- Use
Save current filter
,Your filters
,Filters shared with all users
instead of patterns? Or maybeSave current filter pattern
etc? - Label the Library box
Filter library
orFilter pattern library
? - Instead of
Share pattern
, maybeMake available for all users
? - Instead of
Retain pattern
, maybeRemove 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
- Use
-
-
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 aQuerySet
object. 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? -
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.
-
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 amin
andmax
field, as it is already done for theMammography
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 ?
-
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!
-
Yes, that's what I meant. The
django-filter
module has another range class calledRangeFilter
. 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 ) -
-
-
-
-
-
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.
-
- Log in to comment