Simplify average_chart_inc_histogram_data function in chart_functions.py
Simplifying this code should reduce the risk of issues such as #569.
Comments (31)
-
reporter -
reporter Large reduction in complexity and repetition of one of the chart python functions. References issue
#570→ <<cset 1cf5525d38e3>>
-
reporter Small change to chart JavaScript to ensure that blank chart labels are shown as
Blank
.. References issue#570→ <<cset b15c3d4e86eb>>
-
reporter Reduction in complexity and repetition of histogram calculation - more should be possible. References issue
#570→ <<cset 031625d3e212>>
-
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. -
reporter Removing typo. References issue
#570→ <<cset 5c3d57864061>>
-
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...
-
reporter Your link works perfectly for me. Perhaps you need to clear your browser cache? (Ctrl-F5, perhaps).
-
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!
-
reporter Is there a JavaScript error shown if you right-hand click and then choose "Inspect" (in Chrome)?
-
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)
-
reporter Hmm. It makes no sense to me that you're seeing an error and I'm not.
-
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? ;-)
-
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.
-
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.
-
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...
-
reporter -
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>>
-
reporter @edmcdonagh, I think my new commit will have fixed this. Would you check at some point please? Thanks.
-
reporter Swapping the order (the check that the
name
property exists must be before the check to see if the value of thename
property is undefined. Also fixed a codacy issue. References issue#570→ <<cset 401d09aa4d11>>
-
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...?
-
reporter It's the choice of
mean
,median
orboth
that is causing it to break. It's the bar chart that is causing the failure. Onlymean
works. -
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... -
reporter @edmcdonagh, I've just fixed this for testing.openrem.org by running the
xxxx_update_median_function.py.inactive
migration as0016_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.
-
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.
-
reporter We should be running https://bitbucket.org/openrem/openrem/src/5bfe8f63aa74ec8686cc66de204b586a9277463f/stuff/0002_0_7_fresh_install_add_median.py.inactive?at=develop during the deployment
-
So I should have done that when I set the servers up for ref
#511I 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?
-
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.
-
reporter There's a variable called
median_available
that is currently set to true if the user has postgresql. Iftrue
thenmedian
andboth
appear as options for the chat average. Perhaps this could be tightened to check for postgresql and a workingmedian
annotate. I don't know how to do this at the moment though. -
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. -
reporter - changed status to resolved
Issue is now fixed. Code is much simpler than it was, with less duplication.
- Log in to comment
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>>