Skip to content

[ML] Validate classification dependent_variable cardinality is at lea…#51232

Merged
dimitris-athanasiou merged 5 commits intoelastic:masterfrom
dimitris-athanasiou:validate-classification-dep-var-cardinality-at-least-two
Jan 22, 2020
Merged

[ML] Validate classification dependent_variable cardinality is at lea…#51232
dimitris-athanasiou merged 5 commits intoelastic:masterfrom
dimitris-athanasiou:validate-classification-dep-var-cardinality-at-least-two

Conversation

@dimitris-athanasiou
Copy link
Copy Markdown
Contributor

@dimitris-athanasiou dimitris-athanasiou commented Jan 20, 2020

…st two

Data frame analytics classification currently only supports 2 classes for the
dependent variable. We were checking that the field's cardinality is not higher
than 2 but we should also check it is not less than that as otherwise the process
fails.

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jan 20, 2020

Pinging @elastic/ml-core (:ml)

przemekwitek
przemekwitek approved these changes Jan 20, 2020
View reviewed changes
Copy link
Copy Markdown

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Just a few minor comments

...core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/DataFrameAnalysis.java Outdated
Copy link
Copy Markdown

@przemekwitek przemekwitek Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/limits/constraints
?

...core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/DataFrameAnalysis.java Outdated
Copy link
Copy Markdown

@przemekwitek przemekwitek Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Limits/Constraints
?

.../src/test/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/OutlierDetectionTests.java Outdated
Copy link
Copy Markdown

@przemekwitek przemekwitek Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a "Matchers.empty()" matcher that could be used here.

...n/core/src/test/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/RegressionTests.java Outdated
Copy link
Copy Markdown

@przemekwitek przemekwitek Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a "Matchers.empty()" matcher that could be used here.

dimitris-athanasiou
dimitris-athanasiou commented Jan 21, 2020
View reviewed changes
.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/dataframe/DataFrameAnalyticsManager.java Outdated
Copy link
Copy Markdown
Contributor Author

@dimitris-athanasiou dimitris-athanasiou Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is now necessary as we are checking the field cardinality before we call startAnalytics which refreshes the dest index.

@dimitris-athanasiou
Copy link
Copy Markdown
Contributor Author

dimitris-athanasiou commented Jan 21, 2020

@przemekwitek I have addressed all your points plus fixes a bug regarding refreshing of the dest index which was caught by the tests.

przemekwitek
przemekwitek approved these changes Jan 21, 2020
View reviewed changes
...-multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ClassificationIT.java Outdated
Copy link
Copy Markdown

@przemekwitek przemekwitek Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be reverted?

Copy link
Copy Markdown
Contributor Author

@dimitris-athanasiou dimitris-athanasiou Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes of course.

@przemekwitek
Copy link
Copy Markdown

przemekwitek commented Jan 21, 2020

@przemekwitek I have addressed all your points plus fixes a bug regarding refreshing of the dest index which was caught by the tests.

Please see my comment in ClassificationIT. Otherwise the PR is good to go.

@dimitris-athanasiou
[ML] Validate classification dependent_variable cardinality is at lea…
607fbc4 
…st two

Data frame analytics classification currently only supports 2 classes for the
dependent variable. We were checking that the field's cardinality is not higher
than 2 but we should also check it is not less than that as otherwise the process
fails.
@dimitris-athanasiou
Address review comments
de2c194
@dimitris-athanasiou
Revert accidental change in test and fix another test
b197ed8
@dimitris-athanasiou
Refresh dest before detected extracted fields
283c012
@dimitris-athanasiou dimitris-athanasiou force-pushed the validate-classification-dep-var-cardinality-at-least-two branch from e51393e to eab85d5 Compare January 22, 2020 12:03
@dimitris-athanasiou dimitris-athanasiou merged commit a6fa577 into elastic:master Jan 22, 2020
@dimitris-athanasiou dimitris-athanasiou deleted the validate-classification-dep-var-cardinality-at-least-two branch January 22, 2020 13:47
dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Jan 22, 2020
@dimitris-athanasiou
[7.x][ML] Validate classification dependent_variable cardinality is a…
c84bfbf 
…t lea… (elastic#51232)

Data frame analytics classification currently only supports 2 classes for the
dependent variable. We were checking that the field's cardinality is not higher
than 2 but we should also check it is not less than that as otherwise the process
fails.

Backport of elastic#51232
@dimitris-athanasiou dimitris-athanasiou mentioned this pull request Jan 22, 2020
Merged
dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Jan 22, 2020
@dimitris-athanasiou
[7.6][ML] Validate classification dependent_variable cardinality is a…
849531e 
…t lea… (elastic#51232)

Data frame analytics classification currently only supports 2 classes for the
dependent variable. We were checking that the field's cardinality is not higher
than 2 but we should also check it is not less than that as otherwise the process
fails.

Backport of elastic#51232
@dimitris-athanasiou dimitris-athanasiou mentioned this pull request Jan 22, 2020
Merged
dimitris-athanasiou added a commit that referenced this pull request Jan 22, 2020
@dimitris-athanasiou
[7.x][ML] Validate classification dependent_variable cardinality is a…
59687a9 
…t lea… (#51232) (#51309)

Data frame analytics classification currently only supports 2 classes for the
dependent variable. We were checking that the field's cardinality is not higher
than 2 but we should also check it is not less than that as otherwise the process
fails.

Backport of #51232
dimitris-athanasiou added a commit that referenced this pull request Jan 22, 2020
@dimitris-athanasiou
[7.6][ML] Validate classification dependent_variable cardinality is a…
749123b 
…t lea… (#51232) (#51310)

Data frame analytics classification currently only supports 2 classes for the
dependent variable. We were checking that the field's cardinality is not higher
than 2 but we should also check it is not less than that as otherwise the process
fails.

Backport of #51232
This was referenced Feb 3, 2020
Closed
Closed
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@przemekwitek przemekwitek przemekwitek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>bug :ml Machine learning v7.6.0 v7.7.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dimitris-athanasiou @elasticmachine @przemekwitek @jakelandis