Introduce classification analysis runner. #701

przemekwitek · 2019-09-27T12:13:22Z

Make CDataFrameBoostedTreeRunner class abstract and derive two subclasses from it:

CDataFrameRegressionRunner
CDataFrameClassificationRunner

droberts195 · 2019-10-02T14:06:57Z

I have labelled this >non-issue on the assumption that it will be release-noted via the Java side PR.

tveasey

This looks pretty good. I left some minor comments.

One thing I think is it would be nice to avoid the string comparisons you do reading every row. Basically, you can do this once if you look up the position of the dependent variable in the feature name strings vector (see comments). It would also be good to add a unit test to CDataFrameAnalyzerTest for this behaviour. You can access the data frame CDataFrameAnalyzer creates to do this.

tveasey · 2019-10-02T17:09:05Z

include/api/CDataFrameAnalysisSpecification.h

@@ -173,6 +173,9 @@ class API_EXPORT CDataFrameAnalysisSpecification {
    //!   2. disk is used (only one partition needs to be loaded to main memory)
    void estimateMemoryUsage(CMemoryUsageEstimationResultJsonWriter& writer) const;

+    //! Fills in categorical field names for which empty value should be treated as missing.
+    void columnsForWhichEmptyIsMissing(TStrVec& fieldNames) const;


My inclination would be to pass in the actual field names and return a vector bool which is an indicator function for which columns to treat empty as missing. This will make the check much more efficient, i.e.

Suggested change

void columnsForWhichEmptyIsMissing(TStrVec& fieldNames) const;

TBoolVec columnsForWhichEmptyIsMissing(const TStrVec& fieldNames) const;

tveasey · 2019-10-02T17:10:36Z

include/api/CDataFrameBoostedTreeRunner.h

 private:
    using TBoostedTreeUPtr = std::unique_ptr<maths::CBoostedTree>;
    using TBoostedTreeFactoryUPtr = std::unique_ptr<maths::CBoostedTreeFactory>;
    using TDataSearcherUPtr = CDataFrameAnalysisSpecification::TDataSearcherUPtr;
    using TMemoryEstimator = std::function<void(std::int64_t)>;

+protected:


nit: can we move before private typedefs as per coding standards

tveasey · 2019-10-02T17:21:28Z

lib/api/CDataFrameAnalyzer.cc

@@ -373,13 +384,38 @@ void CDataFrameAnalyzer::writeProgress(int progress,
    writer.flush();
 }

+void mapToVector(const TStrSizeUMap& map, TStrVec& vector) {


Can we move these to local namespace at the start of this file, i.e. these should have local linkage.

tveasey · 2019-10-02T17:22:53Z

lib/api/CDataFrameAnalyzer.cc

+        std::size_t index = entry.second;
+        if (index >= vector.size()) {
+            HANDLE_FATAL(<< "Index out of bounds: " << index);
+        }


move unsafe access in to else in case handle fatal doesn't abort.

Suggested change

}

} else {

tveasey · 2019-10-02T17:23:33Z

lib/api/CDataFrameAnalyzer.cc

+void mapsToVectors(const TStrSizeUMapVec& maps, TStrVecVec& vectors) {
+    assert(vectors.empty());
+    vectors.resize(maps.size());
+    for (std::size_t i = 0; i < maps.size(); i++) {


coding standards

Suggested change

for (std::size_t i = 0; i < maps.size(); i++) {

for (std::size_t i = 0; i < maps.size(); ++i) {

Done.

BTW, I thought this kind of optimization is done automatically by the compiler these days.
But of course I'll comply with the standards :)

For size_t it makes no difference. However, for a user defined type the compiler is forced to call a different function which would usually create a temporary. Since it is just as easy to type ++i as i++ we just mandate that we always use ++i if the return type isn't used to save having to think about it.

tveasey · 2019-10-02T17:38:35Z

lib/api/CDataFrameRegressionRunner.cc

+                                             const TStrVecVec&,
+                                             TRowRef row,
+                                             core::CRapidJsonConcurrentLineWriter& writer) const {
+    const auto& tree{boostedTree()};


As before

Suggested change

const auto& tree{boostedTree()};

const auto& tree = this->boostedTree();

tveasey · 2019-10-02T17:45:21Z

include/api/CDataFrameAnalyzer.h

@@ -104,6 +104,7 @@ class API_EXPORT CDataFrameAnalyzer {
    TDataFrameAnalysisSpecificationUPtr m_AnalysisSpecification;
    TStrVec m_CategoricalFieldNames;
    TStrSizeUMapVec m_CategoricalFieldValues;
+    std::set<std::string> m_EmptyAsMissing;


Suggested change

std::set<std::string> m_EmptyAsMissing;

TBoolVec m_EmptyIsMissing;

tveasey · 2019-10-02T17:50:56Z

lib/api/CDataFrameAnalyzer.cc

+        TStrVec emptyAsMissing;
+        m_AnalysisSpecification->columnsForWhichEmptyIsMissing(emptyAsMissing);
+        m_EmptyAsMissing =
+            std::set<std::string>(emptyAsMissing.begin(), emptyAsMissing.end());


As per my suggestion I'd move this into CDataFrameAnalyzer::captureFieldNames

tveasey · 2019-10-02T17:51:50Z

lib/api/CDataFrameAnalyzer.cc

-            *columns = fieldToValue(isCategorical[i],
-                                    m_CategoricalFieldValues[i], fieldValues[i]);
+            *columns = fieldToValue(isCategorical[i], m_CategoricalFieldValues[i],
+                                    m_EmptyAsMissing.count(m_FieldNames[i]) > 0,


Suggested change

m_EmptyAsMissing.count(m_FieldNames[i]) > 0,

m_EmptyIsMissing[i],

tveasey · 2019-10-02T17:57:21Z

lib/api/CDataFrameBoostedTreeRunner.cc

+}
+
+const CDataFrameBoostedTreeRunner::TBoostedTreeUPtr&
+CDataFrameBoostedTreeRunner::boostedTree() const {
    if (m_BoostedTree == nullptr) {
        HANDLE_FATAL(<< "Internal error: boosted tree object missing. Please report this error.");


If we assert here let's return by constant reference, i.e. const maths::CBoostedTree&.

przemekwitek

This looks pretty good. I left some minor comments.

One thing I think is it would be nice to avoid the string comparisons you do reading every row. Basically, you can do this once if you look up the position of the dependent variable in the feature name strings vector (see comments).

Done.

It would also be good to add a unit test to CDataFrameAnalyzerTest for this behaviour. You can access the data frame CDataFrameAnalyzer creates to do this.

WIP

przemekwitek · 2019-10-03T07:17:05Z

include/api/CDataFrameBoostedTreeRunner.h

 private:
    using TBoostedTreeUPtr = std::unique_ptr<maths::CBoostedTree>;
    using TBoostedTreeFactoryUPtr = std::unique_ptr<maths::CBoostedTreeFactory>;
    using TDataSearcherUPtr = CDataFrameAnalysisSpecification::TDataSearcherUPtr;
    using TMemoryEstimator = std::function<void(std::int64_t)>;

+protected:


przemekwitek · 2019-10-03T07:20:45Z

include/api/CDataFrameAnalyzer.h

@@ -104,6 +104,7 @@ class API_EXPORT CDataFrameAnalyzer {
    TDataFrameAnalysisSpecificationUPtr m_AnalysisSpecification;
    TStrVec m_CategoricalFieldNames;
    TStrSizeUMapVec m_CategoricalFieldValues;
+    std::set<std::string> m_EmptyAsMissing;


przemekwitek · 2019-10-03T07:21:03Z

lib/api/CDataFrameAnalyzer.cc

+        TStrVec emptyAsMissing;
+        m_AnalysisSpecification->columnsForWhichEmptyIsMissing(emptyAsMissing);
+        m_EmptyAsMissing =
+            std::set<std::string>(emptyAsMissing.begin(), emptyAsMissing.end());


przemekwitek · 2019-10-03T07:24:14Z

lib/api/CDataFrameAnalyzer.cc

+void mapsToVectors(const TStrSizeUMapVec& maps, TStrVecVec& vectors) {
+    assert(vectors.empty());
+    vectors.resize(maps.size());
+    for (std::size_t i = 0; i < maps.size(); i++) {


Done.

BTW, I thought this kind of optimization is done automatically by the compiler these days.
But of course I'll comply with the standards :)

przemekwitek · 2019-10-03T07:26:17Z

lib/api/CDataFrameClassificationRunner.cc

+}
+
+const CDataFrameAnalysisConfigReader CDataFrameClassificationRunner::getParameterReader() {
+    CDataFrameAnalysisConfigReader theReader =


przemekwitek · 2019-10-03T07:31:44Z

include/api/CDataFrameAnalysisSpecification.h

@@ -173,6 +173,9 @@ class API_EXPORT CDataFrameAnalysisSpecification {
    //!   2. disk is used (only one partition needs to be loaded to main memory)
    void estimateMemoryUsage(CMemoryUsageEstimationResultJsonWriter& writer) const;

+    //! Fills in categorical field names for which empty value should be treated as missing.
+    void columnsForWhichEmptyIsMissing(TStrVec& fieldNames) const;


przemekwitek · 2019-10-03T07:47:54Z

lib/api/CDataFrameAnalyzer.cc

-            *columns = fieldToValue(isCategorical[i],
-                                    m_CategoricalFieldValues[i], fieldValues[i]);
+            *columns = fieldToValue(isCategorical[i], m_CategoricalFieldValues[i],
+                                    m_EmptyAsMissing.count(m_FieldNames[i]) > 0,


przemekwitek · 2019-10-03T07:48:22Z

lib/api/CDataFrameAnalyzer.cc

+        std::size_t index = entry.second;
+        if (index >= vector.size()) {
+            HANDLE_FATAL(<< "Index out of bounds: " << index);
+        }


przemekwitek · 2019-10-03T07:49:20Z

lib/api/CDataFrameAnalyzer.cc

@@ -373,13 +384,38 @@ void CDataFrameAnalyzer::writeProgress(int progress,
    writer.flush();
 }

+void mapToVector(const TStrSizeUMap& map, TStrVec& vector) {


przemekwitek · 2019-10-03T07:51:08Z

lib/api/CDataFrameBoostedTreeRunner.cc

+}
+
+const CDataFrameBoostedTreeRunner::TBoostedTreeUPtr&
+CDataFrameBoostedTreeRunner::boostedTree() const {
    if (m_BoostedTree == nullptr) {
        HANDLE_FATAL(<< "Internal error: boosted tree object missing. Please report this error.");


tveasey · 2019-10-03T13:32:19Z

lib/api/unittest/CDataFrameAnalyzerTest.cc

@@ -1018,6 +1021,66 @@ void CDataFrameAnalyzerTest::testCategoricalFields() {
    }
 }

+void CDataFrameAnalyzerTest::testCategoricalFields_EmptyAsMissing() {


Suggested change

void CDataFrameAnalyzerTest::testCategoricalFields_EmptyAsMissing() {

void CDataFrameAnalyzerTest::testCategoricalFieldsEmptyAsMissing() {

przemekwitek · 2019-10-03T13:32:31Z

WIP

I've added unit tests. @tveasey, please take another look.

tveasey

Great! Left a couple more minor change requests, but happy for this to go in without any further reviews so I'll go ahead and approve.

tveasey · 2019-10-03T13:32:32Z

lib/api/unittest/CDataFrameAnalyzerTest.cc

@@ -1018,6 +1021,66 @@ void CDataFrameAnalyzerTest::testCategoricalFields() {
    }
 }

+void CDataFrameAnalyzerTest::testCategoricalFields_EmptyAsMissing() {


Suggested change

void CDataFrameAnalyzerTest::testCategoricalFields_EmptyAsMissing() {

void CDataFrameAnalyzerTest::testCategoricalFieldsEmptyAsMissing() {

tveasey · 2019-10-03T13:36:07Z

lib/api/unittest/CDataFrameAnalyzerTest.cc

+        return [expected](double actual) { return expected == actual; };
+    };
+
+    auto nan = []() { return [](double actual) { return isnan(actual); }; };


Can we make this maths::CDataFrameUtils::isMissing? This should match whatever value we use to indicate missing.

Suggested change

auto nan = []() { return [](double actual) { return isnan(actual); }; };

auto missing = []() { return [](double actual) { return maths::CDataFrameUtils::isMissing(actual); }; };

tveasey · 2019-10-03T13:37:16Z

lib/api/unittest/CDataFrameAnalyzerTest.cc

+        assertRow(8, {eq(3.0), eq(1.0), eq(8.0), eq(8.0), eq(0.0)}, rows[8]);
+        assertRow(9, {eq(2.0), eq(2.0), eq(9.0), eq(9.0), eq(1.0)}, rows[9]);
+    });
+}


👍 good test

tveasey · 2019-10-03T13:38:09Z

lib/api/unittest/CDataFrameAnalysisRunnerTest.h

@@ -18,6 +18,8 @@ class CDataFrameAnalysisRunnerTest : public CppUnit::TestFixture {
    void testEstimateMemoryUsage_10();
    void testEstimateMemoryUsage_100();
    void testEstimateMemoryUsage_1000();
+    void testColumnsForWhichEmptyIsMissing_Classification();


Let's stick to camel case we use this in 99.9% of all code (we should fix the other non-standard names in this suite as well, but you don't have to make that change)

Suggested change

void testColumnsForWhichEmptyIsMissing_Classification();

void testColumnsForWhichEmptyIsMissingClassification();

tveasey · 2019-10-03T13:40:24Z

lib/api/unittest/CDataFrameAnalysisRunnerTest.h

@@ -18,6 +18,8 @@ class CDataFrameAnalysisRunnerTest : public CppUnit::TestFixture {
    void testEstimateMemoryUsage_10();
    void testEstimateMemoryUsage_100();
    void testEstimateMemoryUsage_1000();
+    void testColumnsForWhichEmptyIsMissing_Classification();
+    void testColumnsForWhichEmptyIsMissing_Regression();


Suggested change

void testColumnsForWhichEmptyIsMissing_Regression();

void testColumnsForWhichEmptyIsMissingRegression();

tveasey · 2019-10-03T13:42:10Z

lib/api/unittest/CDataFrameAnalysisRunnerTest.cc

+                         bool(emptyAsMissing[3]));
+}
+
+void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissing_Classification() {


Suggested change

void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissing_Classification() {

void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissingClassification() {

tveasey · 2019-10-03T13:42:20Z

lib/api/unittest/CDataFrameAnalysisRunnerTest.cc

+    testColumnsForWhichEmptyIsMissing("classification", true);
+}
+
+void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissing_Regression() {


Suggested change

void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissing_Regression() {

void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissingRegression() {

przemekwitek · 2019-10-03T15:07:22Z

run elasticsearch-ci

…sses from it: CDataFrameRegressionRunner and CDataFrameClassificationRunner.

przemekwitek added the WIP label Sep 27, 2019

przemekwitek force-pushed the classification branch 8 times, most recently from 885a693 to cff6d69 Compare October 2, 2019 08:59

tveasey mentioned this pull request Oct 2, 2019

[ML] Logistic regression loss function for boosted tree training #713

Merged

przemekwitek force-pushed the classification branch 3 times, most recently from 13381dd to d6181ba Compare October 2, 2019 13:47

przemekwitek removed the WIP label Oct 2, 2019

przemekwitek marked this pull request as ready for review October 2, 2019 13:47

przemekwitek added the :ml/DataFrameAnalysis label Oct 2, 2019

przemekwitek mentioned this pull request Oct 2, 2019

[ML] Introduce classification analysis type elastic/elasticsearch#46735

Closed

9 tasks

droberts195 added >non-issue v7.5.0 v8.0.0 labels Oct 2, 2019

przemekwitek force-pushed the classification branch from d6181ba to 88e02b4 Compare October 2, 2019 14:19

tveasey reviewed Oct 2, 2019

View reviewed changes

przemekwitek commented Oct 3, 2019

View reviewed changes

przemekwitek force-pushed the classification branch from 2f0265e to dd86f4e Compare October 3, 2019 13:30

tveasey reviewed Oct 3, 2019

View reviewed changes

tveasey approved these changes Oct 3, 2019

View reviewed changes

przemekwitek force-pushed the classification branch from 77d998e to 7e30490 Compare October 3, 2019 14:19

przemekwitek added 4 commits October 3, 2019 17:08

Make CDataFrameBoostedTreeRunner class abstract and derive two subcla…

6ef21aa

…sses from it: CDataFrameRegressionRunner and CDataFrameClassificationRunner.

Apply review comments

56248b1

Add unit tests

d3bec86

Apply review comments

97ae2ae

przemekwitek force-pushed the classification branch from 7e30490 to 97ae2ae Compare October 3, 2019 15:08

Fix compile error and apply clang-format

be76552

przemekwitek merged commit 0a95ace into elastic:master Oct 4, 2019

przemekwitek deleted the classification branch October 4, 2019 05:05

przemekwitek mentioned this pull request Oct 4, 2019

[7.x] Introduce classification analysis runner. (#701) #714

Merged

przemekwitek added a commit to przemekwitek/ml-cpp that referenced this pull request Oct 4, 2019

Introduce classification analysis runner. (elastic#701)

06007cd

przemekwitek added a commit that referenced this pull request Oct 4, 2019

Introduce classification analysis runner. (#701) (#714)

c19526a

	void columnsForWhichEmptyIsMissing(TStrVec& fieldNames) const;
	TBoolVec columnsForWhichEmptyIsMissing(const TStrVec& fieldNames) const;

	for (std::size_t i = 0; i < maps.size(); i++) {
	for (std::size_t i = 0; i < maps.size(); ++i) {

	const auto& tree{boostedTree()};
	const auto& tree = this->boostedTree();

	std::set<std::string> m_EmptyAsMissing;
	TBoolVec m_EmptyIsMissing;

	m_EmptyAsMissing.count(m_FieldNames[i]) > 0,
	m_EmptyIsMissing[i],

	void CDataFrameAnalyzerTest::testCategoricalFields_EmptyAsMissing() {
	void CDataFrameAnalyzerTest::testCategoricalFieldsEmptyAsMissing() {

	auto nan = []() { return [](double actual) { return isnan(actual); }; };
	auto missing = []() { return [](double actual) { return maths::CDataFrameUtils::isMissing(actual); }; };

	void testColumnsForWhichEmptyIsMissing_Classification();
	void testColumnsForWhichEmptyIsMissingClassification();

	void testColumnsForWhichEmptyIsMissing_Regression();
	void testColumnsForWhichEmptyIsMissingRegression();

	void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissing_Classification() {
	void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissingClassification() {

Introduce classification analysis runner. #701

Introduce classification analysis runner. #701

Conversation

przemekwitek commented Sep 27, 2019 • edited Loading

droberts195 commented Oct 2, 2019

tveasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemekwitek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemekwitek commented Oct 3, 2019

tveasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

przemekwitek commented Oct 3, 2019

przemekwitek commented Sep 27, 2019 •

edited

Loading

przemekwitek left a comment •

edited

Loading