Add a ct_acquisition_type filter to the CT summary view
I would find this useful in order to be able to filter "Spiral" or "Sequenced" acquisitions (I'm currently trying to split high resolution chest CT into these two groups, and can't easily do it at the moment).
Comments (50)
-
reporter -
reporter Removed PX code that is not relevant to this issue. References issue
#679.→ <<cset ebb593428059>>
-
reporter @edmcdonagh, this works as I'd like it to. What do you think about it? I need to write the associated documentation and add it to the changes files. Not sure if I should move the new filter to near the bottom of the filter order?
-
reporter OK. This doesn't work properly. It causes an issue with the chart data for charts at the request or study level. For example, I think that if a study has two spiral acquisitions then the total study DLP is counted twice in study-level charts.
-
reporter Added check for presence of ct_acquisition_type to ensure correct data for charts. This fixes the problem I reported earlier. However, there remains an issue with hyperlinks from histogram bars that I need to address. References issue
#679→ <<cset 67d0a7e3fb34>>
-
reporter Hyperlinks are now sorted. However, there remains an issue with acquisition-level charts - the data isn't correct at the moment. References issue
#679→ <<cset 3630ce1497b6>>
-
reporter Acquisition-level charts are now behaving. I have some tidying to do tomorrow. References issue
#679→ <<cset 918b02ea4c11>>
-
reporter Simplified chart code as all chart now use the same query data. References issue
#679→ <<cset a83061678f4b>>
-
reporter Tidied up. Acquisisition level and request level charts are working, as are links from histgrams. Further testing of other chart types to do. References issue
#679→ <<cset 63c63a33e6ac>>
-
reporter Moved acquisition type filter to the bottom; forced widget to be checkboxes so that it is easy for a user to deselect all the options; styled the options to remove the leading bullet and indent. References issue
#679→ <<cset 87e474f4d840>>
-
reporter My changes to the ct_acq_filter have broken the csv and xlsx exports, and I don't understand why. Nothing is exported if an acquisition type is selected; export works fine if none of the acquisition types are chosen. @edmcdonagh, can you shed any light on why this is the case?
-
I presume they are broken when you test locally? The pipelines are working aren't they?
-
reporter The pipelines work: the branch is on testing.openrem.org. I can't remember how to debug a celery task.
-
We aren't using celery on the testing server... RabbitMQ isn't installed
-
reporter I'm testing it locally - that's where I find the export issues. Exports of the other modalities and CT with no acquisition type filtering work fine. Checking any of the acquisition type checkboxes causes exports to stop working.
-
If you look at the test scripts for exports, you can see how they are called directly without using celery
-
For example https://bitbucket.org/openrem/openrem/src/e506dc19e1737c5fe3fdb66bc2bb2665c0782a1b/openrem/remapp/tests/test_export_ct_xlsx.py?at=develop&fileviewer=file-view-default#test_export_ct_xlsx.py-95 begins to build a filter set then exports. Does that help or not?
-
reporter Thanks - I'll give that a go. I thought implementing the acquisition type filter would be easy...
-
reporter I'm probably being thick, but how do I debug that test in pycharm?
-
reporter I think I've found out: https://docs.djangoproject.com/en/2.1/topics/testing/overview/#running-tests
-
reporter To run a specific test (
test_zero_filter
in the example below) in thetest_export_ct_xlsx.py
file this is the command:python manage.py test remapp.tests.test_export_ct_xlsx.ExportCTxlsx.test_zero_filter
-
reporter I didn't have the xlrd package installed on my local system.
-
reporter Added tests for CT export when ct_acquisition_type is set. References issue
#679→ <<cset 8138ebdb8d38>>
-
reporter Hmm. My tests work fine, but the export still doesn't work if I choose any of the ct_acquisition_type choices. I'm at a loss.
-
I'll take a look this evening and let you know what and how!
-
reporter Updated the way the query is constructed - I think this speeds things up, and could be used for the other modalities. References issue
#679→ <<cset ef9b81d4cc79>>
-
So I haven't got there, but I have made some progress.
The first problem is I think that when the
request.GET
object is passed to the celery task, it is serialized and changes from (abbreviated):request.GET is <QueryDict: {u'requested_procedure': [u''], ... u'plotCTAcquisitionFreq': [u'on'], u'study_dlp_max': [u''], u'ct_acquisition_type': [u'Sequenced Acquisition', u'Stationary Acquisition'], u'patient_age_max': [u''], ... u'model_name': [u'']}> and is of type <class 'django.http.request.QueryDict'>
to:
Request.GET now is {u'requested_procedure': u'',... u'plotCTAcquisitionFreq': u'on', u'study_dlp_max': u'', u'ct_acquisition_type': u'Stationary Acquisition', u'patient_age_max': u'', ... u'model_name': u''} and is of type <type 'dict'>
As you can see, the original QueryDict object, as used with django_filters for the views, is now a standard dict when it has been serialized for passing to Celery. In the QueryDict all the values in the dict were lists, with some (your new multiple choice one) having multiple values; in the dict, the values are all single value strings.
I'm guessing that till now this didn't matter, as everything had only one value? But the new code is expecting a list of strings and getting a string - is that the issue?
-
So, either we serialize differently at https://bitbucket.org/openrem/openrem/src/e506dc19e1737c5fe3fdb66bc2bb2665c0782a1b/openrem/remapp/exports/exportviews.py?at=develop&fileviewer=file-view-default#exportviews.py-113 or we check for the CT acquisition type at that point and create a separate variable to pass?
We need to be able to reconstruct it properly when using it at https://bitbucket.org/openrem/openrem/src/e506dc19e1737c5fe3fdb66bc2bb2665c0782a1b/openrem/remapp/exports/ct_export.py?at=develop&fileviewer=file-view-default#ct_export.py-80
-
reporter Marvellous - thanks for the pointers. I've got it working.
-
reporter Updated export code to ensure that the values of the ct_acquisition_type filter are passed as a list. References issue
#679→ <<cset 86aaf2f344cd>>
-
Brilliant 😁
-
I should add, bug finding was a process of adding various print statements to spot the difference between the display view and the export view... primitive, but I got there!
-
reporter Some optimisation of queries to help with speed. References issue
#679. I'll post some timings in a minute.→ <<cset ca8d5a4a16c2>>
-
reporter Changing from
exp_include = [o.study_instance_uid for o in f]
toto exp_include = f.qs.values_list('study_instance_uid')
increases the speed of things.Radiographic; median DAP per requested procedure name chart; "lumbar" entered in acquisition protocol filter (5324 studies); series per system false:
- Old code: 60 s for chart to appear
- New code: 4 s for chart to appear
CT; median DLP per acquisition protocol chart; "Spiral" acquisition type; "high res" entered in request filter (2110 studies); series per system false; histograms on:
- Old code: 290 s
- New code: 122 s
-
Awesome. Speedups always welcome!
-
reporter Yep. However, I'm not seeing the expected speedup on another test system with this code. Looking into that now.
-
reporter Scrap that - it is working
-
reporter Fixed problem where my exports stopped because of a missing value. References issue
#679→ <<cset 0d3078c07cc0>>
-
reporter Removing prefetch as I wasn't using it appropriately - replaced with values. References issue
#679→ <<cset ad29cbf84f8e>>
-
reporter Updating documents to reflect CT acquisition type filtering [skip ci]. References issue
#679→ <<cset cc1dd473c0cb>>
-
reporter Updated changes files. I want to force a build so that I can test the on-line docs. References issue
#679.→ <<cset 0d51da980def>>
-
reporter Updated screenshot to include CT acquisition type restrictions [skip ci]. References issue
#679→ <<cset 01f0c7008caf>>
-
reporter Changing occurrece of slower [for x in y] code with a distinct values_list as I have done in views.py in this branch. References issue
#679→ <<cset a4beb2299106>>
-
reporter Putting back code I accidentally removed. References issue
#679→ <<cset 7175333f79a8>>
-
reporter Fixing conflicts following merge in of virtual env branch. References issue
#679→ <<cset b29d730ce65a>>
-
reporter Removed unrequired import to appease Codacy. References issue
#679→ <<cset ded7961bf419>>
-
reporter Removed unrequired imports from new tests and added brief comments to each new test. References issue
#679→ <<cset 066d5e61838b>>
-
reporter Removing pointless spaceless tag - this just removes whitespace from between HTML tags. References issue
#679→ <<cset 3d609447e945>>
-
reporter - changed status to resolved
This issue was resolved by pull request #234.
-
- changed milestone to 0.9.0
- Log in to comment
Added a filter for CT acquisition type. References issue
#679.→ <<cset c183a5de1243>>