Add a ct_acquisition_type filter to the CT summary view

Issue #679 resolved
David Platten created an issue

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)

  1. David Platten 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?

  2. David Platten 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.

  3. David Platten 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>>

  4. David Platten 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>>

  5. David Platten 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>>

  6. David Platten 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>>

  7. David Platten 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?

  8. David Platten reporter

    The pipelines work: the branch is on testing.openrem.org. I can't remember how to debug a celery task.

  9. David Platten 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.

  10. Ed McDonagh

    If you look at the test scripts for exports, you can see how they are called directly without using celery

  11. David Platten reporter

    Thanks - I'll give that a go. I thought implementing the acquisition type filter would be easy...

  12. David Platten reporter

    To run a specific test (test_zero_filter in the example below) in the test_export_ct_xlsx.py file this is the command:

    python manage.py test remapp.tests.test_export_ct_xlsx.ExportCTxlsx.test_zero_filter
    
  13. David Platten 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.

  14. David Platten 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>>

  15. Ed McDonagh

    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?

  16. Ed McDonagh
  17. Ed McDonagh

    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!

  18. David Platten reporter

    Changing from exp_include = [o.study_instance_uid for o in f] to to 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
  19. David Platten reporter

    Yep. However, I'm not seeing the expected speedup on another test system with this code. Looking into that now.

  20. David Platten 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>>

  21. Log in to comment