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

Simplify average_chart_inc_histogram_data function in chart_functions.py

Issue #570 resolved
David Platten created an issue

Simplifying this code should reduce the risk of issues such as #569.

Comments (31)

  1. David Platten reporter

    Beginning of simplifying a routine in the chart functions. Needs further work: should be able to reduce the number of if statements significantly and also also the amount of repetition. References issue #570

    → <<cset bd2aea416e44>>

  5. David Platten reporter

    The simplifications that I've made (particularly commit #1cf5525) should reduce the chance of errors in the code such as the one that led to issue #569. The new code has a single section for the calculation of mean, median or both; the previous code had a separate section for each, with a good deal of repetition.

  7. Ed McDonagh

    Does this work for you on testing? I haven't tried to narrow down the issue, but a look at CT with the current charts selected, or http://testing.openrem.org/openrem/ct/?date_after=2014-11-20&date_before=&study_description=&procedure_code_meaning=&requested_procedure=&acquisition_protocol=&patient_age_min=&patient_age_max=&institution_name=&manufacturer=&model_name=&station_name=&accession_number=&study_dlp_min=&study_dlp_max=&display_name=&test_data=&patient_name=&patient_id=&o=-study_date&submit=Submit&csrfmiddlewaretoken=pmcUqufTNmS3TvW1rVIs888fMKFCIynV&plotCharts=on&plotCTStudyMeanDLP=on&plotCTStudyFreq=on&plotCTStudyNumEvents=on&plotCTStudyMeanDLPOverTimePeriod=months&plotMeanMedianOrBoth=mean&plotSeriesPerSystem=on&plotHistograms=on&itemsPerPage=25 to reduce it to the last 50 odd studies never seems to finish the waiting animation...

  8. David Platten reporter

    Your link works perfectly for me. Perhaps you need to clear your browser cache? (Ctrl-F5, perhaps).

  9. Ed McDonagh

    Still doesn't work for me, even with incognito mode. But again, I haven't tried very hard to work out why. Might still just be me!

  10. David Platten reporter

    Is there a JavaScript error shown if you right-hand click and then choose "Inspect" (in Chrome)?

  11. Ed McDonagh 
    Uncaught TypeError: Cannot read property '0' of null
    at sortByName (http://testing.openrem.org/static/js/sorting.js:7:22)
    at Array.sort (native)
    at updateFrequencyChart (http://testing.openrem.org/static/js/chartUpdateData.js:145:18)
    at Object.success (http://testing.openrem.org/static/js/ctChartAjax.js:81:17)
    at j (http://testing.openrem.org/static/js/jquery-1.11.0.min.js:2:27136)
    at Object.fireWith [as resolveWith] (http://testing.openrem.org/static/js/jquery-1.11.0.min.js:2:27949)
    at x (http://testing.openrem.org/static/js/jquery-1.11.0.min.js:4:22244)
    at XMLHttpRequest.b (http://testing.openrem.org/static/js/jquery-1.11.0.min.js:4:26298)
  13. Ed McDonagh

    I get the same on different browsers, different PCs. Firefox error below:

    TypeError: b.name is null
sortByName()
 sorting.js:7
ArraySort/comparefn()
 self-hosted:217
InsertionSort()
 self-hosted:3652
MergeSort()
 self-hosted:3715
ArraySort()
 self-hosted:220
updateFrequencyChart()
 chartUpdateData.js:145
.success()
 ctChartAjax.js:81
n.Callbacks/j()
 jquery-1.11.0.min.js:2
n.Callbacks/k.fireWith()
 jquery-1.11.0.min.js:2
x()
 jquery-1.11.0.min.js:4
.send/b()
 jquery-1.11.0.min.js:4
 sorting.js:7:9

    Clear your browser cache maybe? ;-)

  14. David Platten reporter

    Still works OK for me. I've just rebooted into Ubuntu and installed the "Gnome Web" browser (never used before, so can't have cached things): this works just fine with the link you supplied. Very odd.

  15. Ed McDonagh

    If I select by any of the display names on the home page, no error. If I select all of CT, I get the error every time.

  16. Ed McDonagh

    I've established that if the Toshiba from Oxbridge County Hospital is in the filter, but not on it's own (from 2016-01-01 fro example), then it has the error. Not sure what is going on with that study, as the detail view shows most of the data is missing...

  17. David Platten reporter

    OK. However, that still works for me:

    http://testing.openrem.org/openrem/ct/?date_after=2016-01-01&date_before=&study_description=&procedure_code_meaning=&requested_procedure=&acquisition_protocol=&patient_age_min=&patient_age_max=&institution_name=&manufacturer=&model_name=&station_name=&accession_number=&study_dlp_min=&study_dlp_max=&display_name=&test_data=&patient_name=&patient_id=&o=-study_date&submit=Submit&csrfmiddlewaretoken=v6mCSbKO7A0W8TQp6R9UBO4glHwl9iWq&plotCharts=on&plotCTStudyMeanDLP=on&plotCTStudyFreq=on&plotCTStudyNumEvents=on&plotCTStudyMeanDLPOverTimePeriod=months&plotMeanMedianOrBoth=mean&plotSeriesPerSystem=on&plotHistograms=on&itemsPerPage=25

  18. David Platten reporter

    Added check that the name property exists before trying to sort. Not tested, but I think this is the cause of the problem that Ed has had based on the JavaScript error that he posted. References issue #570

    → <<cset c6f8f8e8486d>>

  19. David Platten reporter

    @edmcdonagh, I think my new commit will have fixed this. Would you check at some point please? Thanks.

  20. David Platten reporter

    Swapping the order (the check that the name property exists must be before the check to see if the value of the name property is undefined. Also fixed a codacy issue. References issue #570

    → <<cset 401d09aa4d11>>

  21. Ed McDonagh

    Hi @dplatten. It seems that if you are logged in as demo it is fine. But if you log in as testingadmin it isn't. I've only looked from my phone, so I don't have the inspect tools, but that seems to be what is happening. No idea why...?

  22. David Platten reporter

    It's the choice of mean, median or both that is causing it to break. It's the bar chart that is causing the failure. Only mean works.

  23. David Platten reporter

    OK. This affects demo.openrem.org, devdemo.openrem.org and testing.openrem.org. I suspect that the PostgreSQL database is missing the median function...

  24. David Platten reporter

    @edmcdonagh, I've just fixed this for testing.openrem.org by running the xxxx_update_median_function.py.inactive migration as 0016_update_median_function.py with the following contents:

    # -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import models, migrations
from django.conf import settings
from django.db.models.loading import get_model


class Migration(migrations.Migration):

    dependencies = [
        ('remapp', '0015_userprofile_itemsperpage'),
    ]

    if 'postgresql' in settings.DATABASES['default']['ENGINE']:
        operations = [
            migrations.RunSQL(
                #"DROP AGGREGATE median(anyelement);"
                #"DROP FUNCTION _final_median(anyarray);"
                "CREATE FUNCTION _final_median(anyarray) RETURNS NUMERIC AS $$"
                "  WITH q AS"
                "  ("
                "     SELECT val"
                "     FROM UNNEST($1) val"
                "     WHERE val IS NOT NULL"
                "     ORDER BY 1"
                "  ),"
                "  cnt AS"
                "  ("
                "    SELECT COUNT(*) AS c FROM q"
                "  )"
                "  SELECT AVG(val)"
                "  FROM"
                "  ("
                "    SELECT val FROM q"
                "    LIMIT  2 - MOD((SELECT c FROM cnt), 2)"
                "    OFFSET GREATEST(CEIL((SELECT c FROM cnt) / 2.0) - 1, 0)"
                "  ) q2;"
                "$$ LANGUAGE SQL IMMUTABLE;"
                "CREATE AGGREGATE median(anyelement) ("
                "  SFUNC=array_append,"
                "  STYPE=anyarray,"
                "  FINALFUNC=_final_median,"
                "  INITCOND='{}'"
                ");",
            ),
        ]

    I'll do the same for the demo site in a minute.

  25. David Platten reporter

    It's now fixed for demo.openrem.org too.

    The code that deploys the various demo sites needs to include adding the median function.

  27. Ed McDonagh

    So I should have done that when I set the servers up for ref #511 I think. Each time we push a new version we just migrate the existing database, so I think I'm right in thinking there is no reason to change anything now? We just need to remember to do it if we set up another database from scratch!

    Is there an easy and obvious way of picking up this error to highlight to the user?

  28. David Platten reporter

    I need to add the median for the dev site.

    The obvious thing for the user is that charts work with mean selected, but not for median or both.

  29. David Platten reporter

    There's a variable called median_available that is currently set to true if the user has postgresql. If true then median and both appear as options for the chat average. Perhaps this could be tightened to check for postgresql and a working median annotate. I don't know how to do this at the moment though.

  30. David Platten reporter

    I would like to close this issue as the commits have resolved it.

    I've created a separate issue (#571) to address the question of more robustly checking whether the Median annotate function really is available on a PostgreSQL-based OpenREM install - it's beyond the scope of this issue.

  32. Log in to comment
Assignee
Type
enhancement
Priority
major
Status
resolved
Component
Interface
Milestone
0.8.0
Votes
0
Watchers
1
Jira: the preferred issue tracker for Bitbucket. Join the team!