[missing values] Reverting replacing missing values with zeros

[john-bodley]

Apologies for not having full context of this code but from a numerical standpoint replacing missing values with zero (or other values) is rarely ever a good idea as this leads to inaccuracies which surely violates the core tenant of a data analysis tool. Note Pandas (implicitly) and Numpy (explicitly) correctly handle missing values, e.g. mean and nanmean respectively.

This PR is still WIP as I've only remedied a couple of the visualization types and still to add a number of unit tests to ensure numerical correctness with missing values. I felt there was merit in sharing this now in order for me to better understand the context of replacing missing values and potential corner cases I need to be aware of.

For context here's a few examples were replacing missing values with 0 leads to incorrect results:

Time-series (current):

Time-series (proposed):

Box-plot (current):

Box-plot (proposed):

Closes #3603

to: @jeffreythewang @mistercrunch @williaster @xrmx

[codecov-io]

Codecov Report

Merging #4905 into master will decrease coverage by 8.69%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4905      +/-   ##
==========================================
- Coverage   64.46%   55.77%    -8.7%     
==========================================
  Files         421      344      -77     
  Lines       20537     9924   -10613     
  Branches     2247     2245       -2     
==========================================
- Hits        13240     5535    -7705     
+ Misses       7170     4262    -2908     
  Partials      127      127

Impacted Files	Coverage Δ
superset/assets/src/explore/controls.jsx	42.4% <ø> (ø)	⬆️
superset/assets/src/components/EditableTitle.jsx	77.14% <0%> (-4.4%)	⬇️
...t/assets/src/dashboard/reducers/getInitialState.js	0% <0%> (ø)	⬆️
.../src/dashboard/components/gridComponents/Chart.jsx	69.73% <0%> (ø)	⬆️
...uperset/assets/src/SqlLab/components/SqlEditor.jsx	57.44% <0%> (ø)	⬆️
...et/assets/src/dashboard/components/SliceHeader.jsx	24% <0%> (ø)	⬆️
...set/assets/src/dashboard/actions/dashboardState.js	15% <0%> (ø)	⬆️
superset/assets/src/dashboard/containers/Chart.jsx	90.9% <0%> (ø)	⬆️
.../assets/src/SqlLab/components/TabbedSqlEditors.jsx	83.17% <0%> (ø)	⬆️
... and 78 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ffdd6...4511846. Read the comment docs.

[williaster]

🙌 why was filling nulls with 0s ever introduced?

[mistercrunch]

I think @xrmx added some of this logic. null-labeled series creates issues on some visualizations. I believe the leading space has to do with sorting NULL first. We may want to reuse some of the logic I added recently around filtering that replaces None by <NULL> and empty stings by <empty string>. This should only be applied to dimensions / strings. Note that it applies only to string columns, thus not "zerofying" NULL values.

I'm pretty sure that removing this will break many charts.

[john-bodley]

@mistercrunch the ' NULL' (with a leading space) is somewhat misleading and is inconsistent with SQL Lab which reports NULL (or missing values) as null. On an unrelated note SQL Lab doesn't seem to sort NULL values correctly and it would be good to ensure that NULL values are rendered/sorted consistency throughout the application.

[mistercrunch]

Are we sure that this particular visualization handles null nicely?

[mistercrunch]

Some visualizations may not handle NULL properly, I'd double check that they do prior to pushing this.

[john-bodley]

@mistercrunch this PR is very much WIP as I need to go through each visualization type and update the logic were necessary. At the moment I've only taken a pass at the box-plot visualization.

The reason I put this PR out was to gain more context (like your comment around ' NULL') to better understand what the logic should be and what tests I should write.

I think there is merit in having consistency between how data in rendered in SQL Lab and slices, i.e., there are inconsistencies here where SQL Lab reports null and slices report NULL (with a leading whitespace which messes with alignment).

[mistercrunch]

Glad you're taking this on! I've had some conversations with @betodealmeida about sorting some of this out.

I'm taking a tangent here thinking about NULL handling in time series. Users may want or expect a set of behaviors:

• break the line, restart it on the other side (highcharts does this I think)
• replace with 0
• link the line across both sides of the null

Option 1 is arguably the best visual representation of what is happening, but as far as I know NVD3 doesn't do this well (maybe it didn't in the past but does now...)

Option 2 makes it pretty clear that data is missing, though clearly NULL is not zero and that's somewhat wrong and ugly (steep diagonals)

Option 3 is misleading, especially in the absence of markers, you may care and never know that data is missing.

Recently we introduced ISO notation to db_engine_specs concept of time grain, which means Superset now knows about the time granularity. This can help us to time resampling and doing proper handling of missing values eventually. Eventually we could have a control section "Handling NULLs" this and offering all the needed options.

[williaster]

I think that option 1 above is the best^ it's super misleading to have nulls replaced with 0s, and if you interpolate between nulls (ie connect them with a line) I think it's just as misleading. users should be able to make the connection between the line segments interrupted by nulls using line color.

[DavidJHassan]

Any progress on this update? Any work arounds to get the "proposed" results seen above?
This is my one and only issue with Superset and would be ecstatic if it was corrected.

More Examples using Line Chart and Table View:

This makes graphs look awful when the data suddenly drops to 0 because of NULLS.
When looking at financial data in a table view it is misleading to 0 instead of NULL. Especially when dealing with prices, sales, etc.

The below row in a table view only contains data in the first estimate column. The other columns are NULL but displays 0's.

[john-bodley]

@DavidHassan apologies for the delay in responding. This is definitely still on my radar though my time has been focused on other Superset related projects of recent. I do hope to address this some time in June. I need to go through and test all the various chart types and ensure they correctly handle the NULL values.

[DavidJHassan]

@john-bodley thank you for taking the time to work on this issue.

[DavidJHassan]

This issue seemed to be resolved for table views in 0.27; However, it is back again in 0.29.rc07.

[john-bodley]

Here is an update using a toy time-series data set which contains missing value and has the following specific features:

1. There is missing values at the beginning of the time-series.
2. There is missing values at the end of the time-series.
3. There are missing values resulting in a single siloed data point as well as adjacent data points.

For context the data set is defined as:

Bellow are examples of the current and proposed charts. Additionally I tested other chart types which could have been potentially impacted by this change.

Line Chart (current)

Line Chart (proposed)

Note that there is an issue with the NDV3 line chart where the siloed data point does not render other than when one hovers over it. This issue will be fixed with the data-ui toolkit.

Pie Chart (current)

Pie Chart (proposed)

Note there is no change here, though the code has been changed to ensure that the NULL values are not dropped in the pivot-table. Internally there was some debate whether this was the right approach, but I sense i) it ensures consistency with other charts, and ii) it accurately represents the underly result set, i.e., it informs the user that a dimension exists however the value is either zero of undefined.

Bar Chart (current)

Bar Chart (proposed)

Note this remains unchanged like the Pie Chart and ensures consistency in terms of defining either zero or undefined values.

Pivot Table (current)

Pivot Table (proposed)

Note there seems to be an issue with any value other than an empty string being rendered as NaN. The current code uses an empty string as the na_rep for pandas.DataFrame.to_html which is inconsistent with the Table visualization type. Additionally I explicitly made Pandas behave like SQL where the summation of only NULLs returns NULL.

to: @kristw @michellethomas @mistercrunch @williaster

[michellethomas]

Sounds good to follow up on max's earlier comments about whether null labeled series creates issues on some visualizations but otherwise this lgtm.

[john-bodley]

Per Wikipedia there are several representations of the whiskers, including the common Tukey boxplot,

The lowest datum still within 1.5 IQR of the lower quartile, and the highest datum still within 1.5 IQR of the upper quartile.

The definition above is the same as the more complex argmin approach and doesn't require a second lookup. The resulting value is in agreement with the definition above.

[kristw]

The proposed changes LGTM. I can add a patch to the pivot_table to handle string 'null' to avoid formatting as NaN.

Have you tried passing null to box plot and see if it breaks anything?

[john-bodley]

Note stdev is not a valid Pandas function.

[mistercrunch]

Happy to see this getting fixed. 🔥