search

chop-dbhi / cilantro

Official client for Harvest (http://harvest.research.chop.edu)
http://cilantro.harvest.io
Other
28 stars 8 forks source link

Distribution charts not rendering #773

Closed bruth closed 8 years ago

bruth commented 8 years ago

This was observed as of 7f65fba63536cb1b0f8188875144b15d7070fac0.

screen shot 2015-11-15 at 9 50 32 am
naegelyd commented 8 years ago

Looking into this now but I'm not certain it is related to https://github.com/chop-dbhi/cilantro/pull/772. Testing locally, the distribution chart does not render correctly on 120f7e13e8f93f8e2b97ba2ae733934a46ad2657 either.

naegelyd commented 8 years ago

Whatever is causing this does not seem to affect all distribution charts. For example, the age chart renders just fine and it is a distribution chart.

screen shot 2015-11-15 at 12 23 37 pm

naegelyd commented 8 years ago

Seems like this might be affecting all the numeric data. Mean Corpuscular Volume (MCV) and White Blood Cells (WBC) also fail to render with a blank chart just like Hemoglobin (HGB)

naegelyd commented 8 years ago

So, after looking at this further, this seems to be an issue in highcharts which is being encountered due to a change in our dist endpoint. First, let's consider the difference in our endpoint then we can look at highcharts.

resources/field/dist.py

In [this commit])https://github.com/chop-dbhi/serrano/commit/4f005a253732762856e44bb08a527bd9e41854c6#diff-021a7c72a335c929a94b4de4d5055630), the dist endpoint was updated to use the DataField.dist method. As a result, the ordering of the returned data changed. Here is a comparison of the data returned data before and after.

Before screen shot 2015-11-15 at 2 28 20 pm screen shot 2015-11-15 at 2 28 37 pm After screen shot 2015-11-15 at 2 27 48 pm

so, we see that after the change, the point with value = null appears first but before the change, the null value was the last point. While we wouldn't think the position of the null value matters, it turns out to be very important. This brings us to highcharts.

Highcharts

highcharts really, really hates nulls in column charts as the first x value. In this JSFiddle we see that an empty chart is rendered when the first data point has null as the first x value. I am not sure why this breaks highcharts but it does as evidenced in that JSFiddle. This JSFiddle shows that the chart behaves just fine so long as the null is any other data point.

I'll put together a PR for this but I think the simplest fix is just to replace null with '' and let highcharts sort it out from there. Here's a JSFiddle showing that behavior with null ==> '' replacement..

bruth commented 8 years ago

That was a pretty epic evaluation. Thanks for digging into it.

naegelyd commented 8 years ago

That was a pretty epic evaluation. Thanks for digging into it.

Yea, I figured the detailed explanation would help convince myself and others. Not sure how believable it would have been if I had just claimed, this endpoint changed the order or returned results and, as a side effect, it broke the charts.