From d581beb8c6c5d90b1fbc069e1b8a345780ae6c4a Mon Sep 17 00:00:00 2001 From: Bela Schaum Date: Wed, 21 Aug 2024 18:21:28 +0200 Subject: [PATCH 1/2] Fix dimension aggregation with NaV value --- CHANGELOG.md | 1 + src/apps/qutils/canvas.cpp | 6 +++++- src/base/geom/circle.cpp | 2 +- src/base/geom/quadrilateral.cpp | 5 +++-- src/base/gfx/pathsampler.cpp | 2 ++ src/chart/rendering/markers/abstractmarker.cpp | 4 ++++ src/chart/rendering/renderedchart.cpp | 2 +- src/dataframe/impl/aggregators.cpp | 9 --------- src/dataframe/impl/data_source.cpp | 11 ++++------- src/dataframe/impl/dataframe.cpp | 4 +--- src/dataframe/interface.h | 8 +------- src/dataframe/old/datatable.cpp | 2 -- test/qtest/chart.cpp | 10 ++++++---- test/unit/base/refl/auto_enum.cpp | 5 +++-- test/unit/chart/events.cpp | 2 ++ test/unit/dataframe/interface_test.cpp | 14 ++------------ test/unit/util/collection.h | 2 +- test/unit/util/condition.h | 3 +-- 18 files changed, 38 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4f6e6f96..b6c9a79dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Area charts with data series started with zero: tooltip fixed. - Series whose contained ',' and aggregated were not working properly. +- Dimension columns with non-existent values aggregation was undefined. ## [0.12.0] - 2024-07-29 diff --git a/src/apps/qutils/canvas.cpp b/src/apps/qutils/canvas.cpp index 0679e12c1..8759492ae 100644 --- a/src/apps/qutils/canvas.cpp +++ b/src/apps/qutils/canvas.cpp @@ -6,6 +6,8 @@ #include #include +// NOLINTBEGIN(misc-include-cleaner,readability-avoid-nested-conditional-operator) + QColor toQColor(const Gfx::Color &color) { return {color.getRedByte(), @@ -276,4 +278,6 @@ QFont BaseCanvas::fromGfxFont(const Gfx::Font &newFont, QFont font) ? QFont::StyleOblique : QFont::StyleNormal); return font; -} \ No newline at end of file +} + +// NOLINTEND(misc-include-cleaner,readability-avoid-nested-conditional-operator) \ No newline at end of file diff --git a/src/base/geom/circle.cpp b/src/base/geom/circle.cpp index 416544904..8f1eb2487 100644 --- a/src/base/geom/circle.cpp +++ b/src/base/geom/circle.cpp @@ -4,8 +4,8 @@ #include #include #include -#include +#include "base/geom/point.h" #include "base/math/floating.h" #include "base/math/tolerance.h" diff --git a/src/base/geom/quadrilateral.cpp b/src/base/geom/quadrilateral.cpp index 96531f538..e3898892e 100644 --- a/src/base/geom/quadrilateral.cpp +++ b/src/base/geom/quadrilateral.cpp @@ -1,9 +1,10 @@ #include "quadrilateral.h" -#include "base/math/tolerance.h" +#include + +#include "base/math/floating.h" #include "point.h" -#include "rect.h" #include "triangle.h" namespace Geom diff --git a/src/base/gfx/pathsampler.cpp b/src/base/gfx/pathsampler.cpp index 89cb01090..a00fb4480 100644 --- a/src/base/gfx/pathsampler.cpp +++ b/src/base/gfx/pathsampler.cpp @@ -1,9 +1,11 @@ #include "pathsampler.h" +#include #include #include "base/geom/point.h" #include "base/geom/triangle.h" +#include "base/math/floating.h" namespace Gfx { diff --git a/src/chart/rendering/markers/abstractmarker.cpp b/src/chart/rendering/markers/abstractmarker.cpp index d96db0e73..090f5b69a 100644 --- a/src/chart/rendering/markers/abstractmarker.cpp +++ b/src/chart/rendering/markers/abstractmarker.cpp @@ -1,5 +1,9 @@ #include "abstractmarker.h" +#include +#include +#include + #include "base/anim/interpolated.h" #include "base/geom/line.h" #include "chart/generator/marker.h" diff --git a/src/chart/rendering/renderedchart.cpp b/src/chart/rendering/renderedchart.cpp index 4e4c25488..61a2f64a2 100644 --- a/src/chart/rendering/renderedchart.cpp +++ b/src/chart/rendering/renderedchart.cpp @@ -1,13 +1,13 @@ #include "renderedchart.h" #include +#include #include #include #include "base/geom/point.h" #include "base/geom/transformedrect.h" #include "base/math/floating.h" -#include "base/math/fuzzybool.h" #include "base/util/eventdispatcher.h" #include "chart/generator/marker.h" // NOLINT(misc-include-cleaner) #include "chart/options/shapetype.h" diff --git a/src/dataframe/impl/aggregators.cpp b/src/dataframe/impl/aggregators.cpp index a455afc50..fbcb55d2d 100644 --- a/src/dataframe/impl/aggregators.cpp +++ b/src/dataframe/impl/aggregators.cpp @@ -114,15 +114,6 @@ get_aggregators() noexcept *std::get_if(&cell)) set.insert(v); return static_cast(set.size()); - }}, - {aggrs[static_cast(aggregator_type::exists)], - empty_double, - [](custom_aggregator::id_type &id, - cell_reference const &cell) -> double - { - auto &b = *std::get_if(&id); - if (is_valid(cell)) b = 1.0; - return b; }}}}}; } diff --git a/src/dataframe/impl/data_source.cpp b/src/dataframe/impl/data_source.cpp index 1e8eeff70..98c4e4f2a 100644 --- a/src/dataframe/impl/data_source.cpp +++ b/src/dataframe/impl/data_source.cpp @@ -238,14 +238,11 @@ cell_reference data_source::get_data(std::size_t record_id, using enum series_type; case dimension: { const auto &dims = unsafe_get(series).second; - if (record_id >= dims.values.size()) return nullptr; return dims.get(record_id); } case measure: { const auto &meas = unsafe_get(series).second; - if (record_id >= meas.values.size()) return nan; - - return meas.values[record_id]; + return meas.get(record_id); } case unknown: default: error(error_type::series_not_found, column); @@ -627,9 +624,9 @@ void data_source::dimension_t::add_more_data( const std::string *data_source::dimension_t::get( std::size_t index) const { - return values[index] == nav - ? nullptr - : std::addressof(categories[values[index]]); + return index < values.size() && values[index] != nav + ? std::addressof(categories[values[index]]) + : nullptr; } void data_source::dimension_t::set(std::size_t index, diff --git a/src/dataframe/impl/dataframe.cpp b/src/dataframe/impl/dataframe.cpp index 42bdab5f9..f125cf079 100644 --- a/src/dataframe/impl/dataframe.cpp +++ b/src/dataframe/impl/dataframe.cpp @@ -94,9 +94,7 @@ dataframe::dataframe(std::shared_ptr other, void valid_unexistent_aggregator(const std::string_view &series, const dataframe::any_aggregator_type &agg) { - if (!agg - || (*agg != aggregator_type::count - && *agg != aggregator_type::exists)) + if (!agg || *agg != aggregator_type::count) error(error_type::series_not_found, series); } diff --git a/src/dataframe/interface.h b/src/dataframe/interface.h index b14c70b7f..04e29b181 100644 --- a/src/dataframe/interface.h +++ b/src/dataframe/interface.h @@ -27,8 +27,7 @@ enum class aggregator_type : std::uint8_t { max, mean, count, - distinct, - exists + distinct }; enum class sort_type : std::uint8_t { @@ -63,11 +62,6 @@ struct custom_aggregator return name <=> oth.name; } - auto operator!=(const custom_aggregator &oth) const - { - return name != oth.name; - } - auto operator==(const custom_aggregator &oth) const { return name == oth.name; diff --git a/src/dataframe/old/datatable.cpp b/src/dataframe/old/datatable.cpp index d8f00852f..6c215b276 100644 --- a/src/dataframe/old/datatable.cpp +++ b/src/dataframe/old/datatable.cpp @@ -16,10 +16,8 @@ #include "base/conv/auto_json.h" #include "base/conv/numtostr.h" #include "base/refl/auto_enum.h" -#include "base/text/funcstring.h" #include "chart/options/options.h" #include "chart/options/shapetype.h" -#include "dataframe/impl/aggregators.h" #include "dataframe/impl/data_source.h" #include "dataframe/interface.h" diff --git a/test/qtest/chart.cpp b/test/qtest/chart.cpp index 9fcadf5e5..2800af188 100644 --- a/test/qtest/chart.cpp +++ b/test/qtest/chart.cpp @@ -146,10 +146,9 @@ void TestChart::run() IO::log() << "step 1b"; auto &options = chart.getChart().getOptions(); auto &styles = chart.getChart().getStyles(); - options.dataFilter = - Vizzu::Data::Filter{std::shared_ptr{ - std::shared_ptr{}, + options.dataFilter = Vizzu::Data::Filter{ + std::unique_ptr{ +[](const Vizzu::Data::RowWrapper *row) -> bool { return *std::get( @@ -158,6 +157,9 @@ void TestChart::run() || *std::get( row->get_value("Cat2")) == std::string_view{"b"}; + }, + +[](bool(const Vizzu::Data::RowWrapper *)) + { }}}; options.title = "VIZZU Chart - Phase 1b"; styles.legend.marker.type = diff --git a/test/unit/base/refl/auto_enum.cpp b/test/unit/base/refl/auto_enum.cpp index 0455dcede..e3f4e0c80 100644 --- a/test/unit/base/refl/auto_enum.cpp +++ b/test/unit/base/refl/auto_enum.cpp @@ -38,7 +38,7 @@ template std::string toString(T v) { return Refl::enum_name(v); } -template T parse(std::string s) +template T parse(const std::string &s) { return Refl::get_enum(s); } @@ -120,7 +120,8 @@ const static auto tests = { throws() << [] { - return Refl::enum_name(Foo::fobar{2}); + return Refl::enum_name( + std::bit_cast(2)); }; }) diff --git a/test/unit/chart/events.cpp b/test/unit/chart/events.cpp index 4d613aaac..324278955 100644 --- a/test/unit/chart/events.cpp +++ b/test/unit/chart/events.cpp @@ -55,6 +55,7 @@ struct MyCanvas final : Gfx::ICanvas, Vizzu::Draw::Painter void frameBegin() final {} void frameEnd() final {} void *getPainter() final { return static_cast(this); } + // cppcheck-suppress duplInheritedMember ICanvas &getCanvas() final { return *this; } }; @@ -333,6 +334,7 @@ std::multimap> get_events( "draw-" + std::string{s.back()}); } + // cppcheck-suppress uselessCallsConstructor if (s.back() == "base") s = {s.begin(), s.end() - 1}; std::string name; diff --git a/test/unit/dataframe/interface_test.cpp b/test/unit/dataframe/interface_test.cpp index f06029028..d1f94eb76 100644 --- a/test/unit/dataframe/interface_test.cpp +++ b/test/unit/dataframe/interface_test.cpp @@ -57,6 +57,7 @@ struct if_setup skip->*df->get_dimensions() == dims; skip->*df->get_measures() == meas; + // cppcheck-suppress ignoredReturnValue skip->*df->get_record_count() == data.size(); for (auto r = 0u; r < data.size(); ++r) { @@ -77,6 +78,7 @@ struct if_setup std::get(df->get_data(r, meas[m])); auto &&nData = data[r][m + ds]; if (nData && std::isnan(gdata)) continue; + // cppcheck-suppress ignoredReturnValue skip->*gdata == std::stod(data[r][m + ds]); } } @@ -484,13 +486,11 @@ const static auto tests = auto &&pure1c = df->set_aggregate({}, count); auto &&d1c = df->set_aggregate("d1", count); auto &&d1d = df->set_aggregate("d1", distinct); - auto &&d1e = df->set_aggregate("d1", exists); auto &&m1s = df->set_aggregate("m1", sum); auto &&m1mi = df->set_aggregate("m1", min); auto &&m1ma = df->set_aggregate("m1", max); auto &&m1me = df->set_aggregate("m1", mean); auto &&m1c = df->set_aggregate("m1", count); - auto &&m1e = df->set_aggregate("m1", exists); /* auto &&m1t = df->set_aggregate("m1", @@ -524,8 +524,6 @@ auto &&m1t = df->set_aggregate("m1", d1c, m1c, d1d, - d1e, - m1e, m1s, m1ma, m1me, @@ -539,8 +537,6 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{0}, d1c) == 5.0; check->*df->get_data(std::size_t{0}, m1c) == 5.0; check->*df->get_data(std::size_t{0}, d1d) == 1.0; - check->*df->get_data(std::size_t{0}, d1e) == 1.0; - check->*df->get_data(std::size_t{0}, m1e) == 1.0; check->*df->get_data(std::size_t{0}, m1ma) == 88.0; check->*df->get_data(std::size_t{0}, m1me) == 21.85; check->*df->get_data(std::size_t{0}, m1mi) == 2.0; @@ -551,8 +547,6 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{1}, d1c) == 4.0; check->*df->get_data(std::size_t{1}, m1c) == 3.0; check->*df->get_data(std::size_t{1}, d1d) == 1.0; - check->*df->get_data(std::size_t{1}, d1e) == 1.0; - check->*df->get_data(std::size_t{1}, m1e) == 1.0; check->*df->get_data(std::size_t{1}, m1ma) == 7.25; check->*df->get_data(std::size_t{1}, m1me) == 5.0; check->*df->get_data(std::size_t{1}, m1mi) == 3.5; @@ -563,8 +557,6 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{2}, d1c) == 1.0; check->*df->get_data(std::size_t{2}, m1c) == 0.0; check->*df->get_data(std::size_t{2}, d1d) == 1.0; - check->*df->get_data(std::size_t{2}, d1e) == 1.0; - check->*df->get_data(std::size_t{2}, m1e) == 0.0; check ->*std::isnan(std::get( df->get_data(std::size_t{2}, m1ma))) @@ -585,8 +577,6 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{3}, d1c) == 0.0; check->*df->get_data(std::size_t{3}, m1c) == 1.0; check->*df->get_data(std::size_t{3}, d1d) == 0.0; - check->*df->get_data(std::size_t{3}, d1e) == 0.0; - check->*df->get_data(std::size_t{3}, m1e) == 1.0; } | "aggregate multiple dim" | diff --git a/test/unit/util/collection.h b/test/unit/util/collection.h index b2c0917e4..742279a98 100644 --- a/test/unit/util/collection.h +++ b/test/unit/util/collection.h @@ -101,7 +101,7 @@ class collection : public case_registry return stats; } - void print_summary(statistics stats) + void print_summary(const statistics &stats) { std::cout << "\n" << "all tests: " << cases.size() << "\n" diff --git a/test/unit/util/condition.h b/test/unit/util/condition.h index bc7ef0c64..fb245b73a 100644 --- a/test/unit/util/condition.h +++ b/test/unit/util/condition.h @@ -20,9 +20,8 @@ template struct decomposer template void operator==(const U &ref) const { - if constexpr (requires { ref == value; }) { + if constexpr (requires { ref == value; }) return evaluate(value == ref, "==", ref); - } else if constexpr (std::ranges::range) { auto &&[lhs, rhs] = std::ranges::mismatch(value, ref); return evaluate(lhs == std::end(value) From 846bf5d086c78d0666420923dfed38ea4c954759 Mon Sep 17 00:00:00 2001 From: Bela Schaum Date: Wed, 21 Aug 2024 18:35:29 +0200 Subject: [PATCH 2/2] Revert exists removing --- src/dataframe/impl/aggregators.cpp | 9 +++++++++ src/dataframe/impl/dataframe.cpp | 4 +++- src/dataframe/interface.h | 3 ++- test/unit/dataframe/interface_test.cpp | 12 ++++++++++++ 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/dataframe/impl/aggregators.cpp b/src/dataframe/impl/aggregators.cpp index fbcb55d2d..a455afc50 100644 --- a/src/dataframe/impl/aggregators.cpp +++ b/src/dataframe/impl/aggregators.cpp @@ -114,6 +114,15 @@ get_aggregators() noexcept *std::get_if(&cell)) set.insert(v); return static_cast(set.size()); + }}, + {aggrs[static_cast(aggregator_type::exists)], + empty_double, + [](custom_aggregator::id_type &id, + cell_reference const &cell) -> double + { + auto &b = *std::get_if(&id); + if (is_valid(cell)) b = 1.0; + return b; }}}}}; } diff --git a/src/dataframe/impl/dataframe.cpp b/src/dataframe/impl/dataframe.cpp index f125cf079..42bdab5f9 100644 --- a/src/dataframe/impl/dataframe.cpp +++ b/src/dataframe/impl/dataframe.cpp @@ -94,7 +94,9 @@ dataframe::dataframe(std::shared_ptr other, void valid_unexistent_aggregator(const std::string_view &series, const dataframe::any_aggregator_type &agg) { - if (!agg || *agg != aggregator_type::count) + if (!agg + || (*agg != aggregator_type::count + && *agg != aggregator_type::exists)) error(error_type::series_not_found, series); } diff --git a/src/dataframe/interface.h b/src/dataframe/interface.h index 04e29b181..d37456173 100644 --- a/src/dataframe/interface.h +++ b/src/dataframe/interface.h @@ -27,7 +27,8 @@ enum class aggregator_type : std::uint8_t { max, mean, count, - distinct + distinct, + exists }; enum class sort_type : std::uint8_t { diff --git a/test/unit/dataframe/interface_test.cpp b/test/unit/dataframe/interface_test.cpp index d1f94eb76..cec18038a 100644 --- a/test/unit/dataframe/interface_test.cpp +++ b/test/unit/dataframe/interface_test.cpp @@ -486,11 +486,13 @@ const static auto tests = auto &&pure1c = df->set_aggregate({}, count); auto &&d1c = df->set_aggregate("d1", count); auto &&d1d = df->set_aggregate("d1", distinct); + auto &&d1e = df->set_aggregate("d1", exists); auto &&m1s = df->set_aggregate("m1", sum); auto &&m1mi = df->set_aggregate("m1", min); auto &&m1ma = df->set_aggregate("m1", max); auto &&m1me = df->set_aggregate("m1", mean); auto &&m1c = df->set_aggregate("m1", count); + auto &&m1e = df->set_aggregate("m1", exists); /* auto &&m1t = df->set_aggregate("m1", @@ -524,6 +526,8 @@ auto &&m1t = df->set_aggregate("m1", d1c, m1c, d1d, + d1e, + m1e, m1s, m1ma, m1me, @@ -537,6 +541,8 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{0}, d1c) == 5.0; check->*df->get_data(std::size_t{0}, m1c) == 5.0; check->*df->get_data(std::size_t{0}, d1d) == 1.0; + check->*df->get_data(std::size_t{0}, d1e) == 1.0; + check->*df->get_data(std::size_t{0}, m1e) == 1.0; check->*df->get_data(std::size_t{0}, m1ma) == 88.0; check->*df->get_data(std::size_t{0}, m1me) == 21.85; check->*df->get_data(std::size_t{0}, m1mi) == 2.0; @@ -547,6 +553,8 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{1}, d1c) == 4.0; check->*df->get_data(std::size_t{1}, m1c) == 3.0; check->*df->get_data(std::size_t{1}, d1d) == 1.0; + check->*df->get_data(std::size_t{1}, d1e) == 1.0; + check->*df->get_data(std::size_t{1}, m1e) == 1.0; check->*df->get_data(std::size_t{1}, m1ma) == 7.25; check->*df->get_data(std::size_t{1}, m1me) == 5.0; check->*df->get_data(std::size_t{1}, m1mi) == 3.5; @@ -557,6 +565,8 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{2}, d1c) == 1.0; check->*df->get_data(std::size_t{2}, m1c) == 0.0; check->*df->get_data(std::size_t{2}, d1d) == 1.0; + check->*df->get_data(std::size_t{2}, d1e) == 1.0; + check->*df->get_data(std::size_t{2}, m1e) == 0.0; check ->*std::isnan(std::get( df->get_data(std::size_t{2}, m1ma))) @@ -577,6 +587,8 @@ auto &&m1t = df->set_aggregate("m1", check->*df->get_data(std::size_t{3}, d1c) == 0.0; check->*df->get_data(std::size_t{3}, m1c) == 1.0; check->*df->get_data(std::size_t{3}, d1d) == 0.0; + check->*df->get_data(std::size_t{3}, d1e) == 0.0; + check->*df->get_data(std::size_t{3}, m1e) == 1.0; } | "aggregate multiple dim" |