From 5f2a8d6c9ae65ac7f9b935b8cb30dba249cb2424 Mon Sep 17 00:00:00 2001 From: Andrew Stein Date: Tue, 28 Aug 2018 13:47:52 -0400 Subject: [PATCH] Fixed bug in mean aggregate which skipped 0 in addition to unset. --- packages/perspective/src/cpp/gnode_state.cpp | 44 +++++++++++++++---- packages/perspective/src/cpp/main.cpp | 2 +- packages/perspective/src/cpp/sparse_tree.cpp | 17 ++----- .../src/include/perspective/gnode_state.h | 5 +++ packages/perspective/src/js/perspective.js | 5 ++- packages/perspective/test/js/pivots.js | 28 +++++++++++- 6 files changed, 74 insertions(+), 27 deletions(-) diff --git a/packages/perspective/src/cpp/gnode_state.cpp b/packages/perspective/src/cpp/gnode_state.cpp index 79edbb7d05..e9feeee67d 100644 --- a/packages/perspective/src/cpp/gnode_state.cpp +++ b/packages/perspective/src/cpp/gnode_state.cpp @@ -490,22 +490,50 @@ void t_gstate::read_column(const t_str& colname, const t_tscalvec& pkeys, t_f64vec& out_data) const +{ + read_column(colname, pkeys, out_data, true); +} + +void +t_gstate::read_column(const t_str& colname, + const t_tscalvec& pkeys, + t_f64vec& out_data, + bool include_nones) const { t_index num = pkeys.size(); t_col_csptr col = m_table->get_const_column(colname); const t_column* col_ = col.get(); - t_f64vec rval(num); - - for (t_index idx = 0; idx < num; ++idx) + + if (include_nones) { - t_mapping::const_iterator iter = m_mapping.find(pkeys[idx]); - if (iter != m_mapping.end()) + t_f64vec rval(num); + for (t_index idx = 0; idx < num; ++idx) { - rval[idx] = col_->get_scalar(iter->second).to_double(); + t_mapping::const_iterator iter = m_mapping.find(pkeys[idx]); + if (iter != m_mapping.end()) + { + rval[idx] = col_->get_scalar(iter->second).to_double(); + } } + std::swap(rval, out_data); } - - std::swap(rval, out_data); + else + { + t_f64vec rval; + for (t_index idx = 0; idx < num; ++idx) + { + t_mapping::const_iterator iter = m_mapping.find(pkeys[idx]); + if (iter != m_mapping.end()) + { + auto tscalar = col_->get_scalar(iter->second); + if (tscalar.is_valid()) + { + rval.push_back(tscalar.to_double()); + } + } + } + std::swap(rval, out_data); + } } t_tscalar diff --git a/packages/perspective/src/cpp/main.cpp b/packages/perspective/src/cpp/main.cpp index 4eb9249435..8cf135f8df 100644 --- a/packages/perspective/src/cpp/main.cpp +++ b/packages/perspective/src/cpp/main.cpp @@ -256,7 +256,7 @@ _fill_col(val dcol, t_col_sptr col, t_bool is_arrow) } else { for (auto i = 0; i < nrows; ++i) { - if (dcol[i].isUndefined()) continue; + if (dcol[i].isUndefined() || dcol[i].isNull()) continue; auto elem = dcol[i].as(); col->set_nth(i, elem); } diff --git a/packages/perspective/src/cpp/sparse_tree.cpp b/packages/perspective/src/cpp/sparse_tree.cpp index d53575b8c3..d113246073 100644 --- a/packages/perspective/src/cpp/sparse_tree.cpp +++ b/packages/perspective/src/cpp/sparse_tree.cpp @@ -1204,22 +1204,11 @@ t_stree::update_agg_table(t_uindex nidx, t_f64vec values; gstate.read_column( - spec.get_dependencies()[0].name(), pkeys, values); + spec.get_dependencies()[0].name(), pkeys, values, false); - t_float64 nr = 0; - t_float64 dr = 0; - auto bit = values.begin(); - auto eit = values.end(); + auto nr = std::accumulate(values.begin(), values.end(), t_float64(0)); + t_float64 dr = values.size(); - for(; bit != eit; ++bit) - { - auto val = *bit; - if (val != NULL) - { - nr += val; - dr += 1; - } - } t_f64pair* dst_pair = dst->get_nth(dst_ridx); diff --git a/packages/perspective/src/include/perspective/gnode_state.h b/packages/perspective/src/include/perspective/gnode_state.h index 10cab0baab..423b0790ca 100644 --- a/packages/perspective/src/include/perspective/gnode_state.h +++ b/packages/perspective/src/include/perspective/gnode_state.h @@ -56,6 +56,11 @@ class PERSPECTIVE_EXPORT t_gstate const t_tscalvec& pkeys, t_f64vec& out_data) const; + void read_column(const t_str& colname, + const t_tscalvec& pkeys, + t_f64vec& out_data, + bool include_nones) const; + t_table_sptr get_table(); t_table_csptr get_table() const; diff --git a/packages/perspective/src/js/perspective.js b/packages/perspective/src/js/perspective.js index 671e311485..c46522d2f3 100644 --- a/packages/perspective/src/js/perspective.js +++ b/packages/perspective/src/js/perspective.js @@ -186,8 +186,9 @@ function parse_data(data, names, types) { if (inferredType.value === __MODULE__.t_dtype.DTYPE_FLOAT64.value) { col.push(Number(data[x][name])); } else if (inferredType.value === __MODULE__.t_dtype.DTYPE_INT32.value) { - const val = Number(data[x][name]); - col.push(val); + let val = data[x][name]; + if (val !== null) val = Number(val); + col.push(val); if (val > 2147483647 || val < -2147483648) { types[n] = __MODULE__.t_dtype.DTYPE_FLOAT64; } diff --git a/packages/perspective/test/js/pivots.js b/packages/perspective/test/js/pivots.js index f44c77ef37..c3f0562db4 100644 --- a/packages/perspective/test/js/pivots.js +++ b/packages/perspective/test/js/pivots.js @@ -156,7 +156,7 @@ module.exports = (perspective) => { describe("Aggregates with nulls", function () { - it("Mean", async function () { + it("mean", async function () { var table = perspective.table([ {'x': 3, 'y':1}, {'x': 2, 'y':1}, @@ -169,7 +169,7 @@ module.exports = (perspective) => { row_pivot: ['y'], aggregate: [{op: 'mean', column:'x'}], }); - var answer = [ + var answer = [ {__ROW_PATH__: [], x: 3}, {__ROW_PATH__: [ 1 ], x: 2.5}, {__ROW_PATH__: [ 2 ], x: 4}, @@ -178,6 +178,30 @@ module.exports = (perspective) => { expect(answer).toEqual(result); }); + it("mean with 0", async function () { + var table = perspective.table([ + {'x': 3, 'y':1}, + {'x': 3, 'y':1}, + {'x': 0, 'y':1}, + {'x': null, 'y':1}, + {'x': null, 'y':1}, + {'x': 4, 'y':2}, + {'x': null, 'y':2} + ]); + var view = table.view({ + row_pivot: ['y'], + aggregate: [{op: 'mean', column:'x'}], + }); + var answer = [ + {__ROW_PATH__: [], x: 2.5}, + {__ROW_PATH__: [ 1 ], x: 2}, + {__ROW_PATH__: [ 2 ], x: 4}, + ]; + let result = await view.to_json(); + expect(answer).toEqual(result); + }); + + it("sum", async function () { var table = perspective.table([ {'x': 3, 'y':1},