Introduce classification analysis runner. by przemekwitek · Pull Request #701 · elastic/ml-cpp

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

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

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

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

lib/api/CDataFrameAnalyzer.cc

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

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

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

As before

Suggested change

const auto& tree{boostedTree()};

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

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

include/api/CDataFrameAnalyzer.h

Suggested change

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

TBoolVec m_EmptyIsMissing;

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

lib/api/CDataFrameAnalyzer.cc

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

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

lib/api/CDataFrameAnalyzer.cc

Suggested change

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

m_EmptyIsMissing[i],

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

lib/api/CDataFrameBoostedTreeRunner.cc

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

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

include/api/CDataFrameAnalyzer.h

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

lib/api/CDataFrameAnalyzer.cc

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

lib/api/CDataFrameAnalyzer.cc

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

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

include/api/CDataFrameAnalysisSpecification.h

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

lib/api/CDataFrameAnalyzer.cc

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

lib/api/CDataFrameAnalyzer.cc

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

lib/api/CDataFrameAnalyzer.cc

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

lib/api/CDataFrameBoostedTreeRunner.cc

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

lib/api/unittest/CDataFrameAnalyzerTest.cc

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

Suggested change

void CDataFrameAnalyzerTest::testCategoricalFields_EmptyAsMissing() {

void CDataFrameAnalyzerTest::testCategoricalFieldsEmptyAsMissing() {

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

lib/api/unittest/CDataFrameAnalyzerTest.cc

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

👍 good test

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

lib/api/unittest/CDataFrameAnalysisRunnerTest.h

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

Suggested change

void testColumnsForWhichEmptyIsMissing_Regression();

void testColumnsForWhichEmptyIsMissingRegression();

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

lib/api/unittest/CDataFrameAnalysisRunnerTest.cc

Suggested change

void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissing_Classification() {

void CDataFrameAnalysisRunnerTest::testColumnsForWhichEmptyIsMissingClassification() {

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

lib/api/unittest/CDataFrameAnalysisRunnerTest.cc

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

Przemyslaw Witek 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 pushed a commit to przemekwitek/ml-cpp that referenced this pull request Oct 4, 2019

Introduce classification analysis runner. (elastic#701)

06007cd

przemekwitek pushed 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() {

Conversation

przemekwitek commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droberts195 commented Oct 2, 2019

Uh oh!

tveasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

przemekwitek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

przemekwitek commented Oct 3, 2019

przemekwitek commented Sep 27, 2019 •

edited

Loading

przemekwitek left a comment •

edited

Loading