Skip to content

Change format of MulticlassConfusionMatrix result to be more self-explanatory #48174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Oct 21, 2019
Merged

Change format of MulticlassConfusionMatrix result to be more self-explanatory #48174

merged 6 commits into from
Oct 21, 2019

Conversation

@przemekwitek
Copy link
Contributor

@przemekwitek przemekwitek commented Oct 17, 2019

Current MulticlassConfusionMatrix result can be confusing to the API user.
They have no means to tell if the outer map is actual or predicted. Similarly for inner map.

This PR fixes that by making the format strict (naming each field).

Relates #46735

@przemekwitek przemekwitek force-pushed the revamp_multiclass_confusion_matrix branch from 66e1005 to 071cbd7 Compare October 17, 2019 08:45
@przemekwitek przemekwitek removed the WIP label Oct 17, 2019
@przemekwitek przemekwitek force-pushed the revamp_multiclass_confusion_matrix branch from 071cbd7 to fa6ab5b Compare October 17, 2019 08:52
@przemekwitek przemekwitek marked this pull request as ready for review October 17, 2019 08:53
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 17, 2019

Pinging @elastic/ml-core (:ml)

@przemekwitek
Copy link
Contributor Author

przemekwitek commented Oct 17, 2019

run elasticsearch-ci/packaging-sample-matrix

@przemekwitek przemekwitek force-pushed the revamp_multiclass_confusion_matrix branch 4 times, most recently from 4ce3aa3 to 97628e2 Compare October 18, 2019 07:11
@przemekwitek
Copy link
Contributor Author

przemekwitek commented Oct 18, 2019 •
edited
Loading

@elasticmachine update branch

1 similar comment
@przemekwitek
Copy link
Contributor Author

przemekwitek commented Oct 18, 2019

@elasticmachine update branch

@przemekwitek przemekwitek force-pushed the revamp_multiclass_confusion_matrix branch 2 times, most recently from 2fd6087 to dfecdf6 Compare October 18, 2019 08:56
@benwtrent benwtrent self-requested a review October 18, 2019 13:17
benwtrent
benwtrent reviewed Oct 18, 2019
View reviewed changes
Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

I really like the new format. It also gives us flexibility to add new fields in the future if we deem them necessary.

...ticsearch/client/ml/dataframe/evaluation/classification/MulticlassConfusionMatrixMetric.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

According to the ctor, both of these are nullable.

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...ticsearch/client/ml/dataframe/evaluation/classification/MulticlassConfusionMatrixMetric.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

Should this be nullable? I think it would throw if predictedClasses == null

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

might be good to do a otherActualClassCount >= 0 check.

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

Suggested change
this.otherActualClassCount = in.readLong();
this.otherActualClassCount = in.readVLong();

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

Suggested change
out.writeLong(actualClassDocCount);
out.writeVLong(actualClassDocCount);

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

Suggested change
this.count = in.readLong();
this.count = in.readVLong();

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

Suggested change
out.writeLong(count);
out.writeVLong(count);

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

null check?

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

...asticsearch/xpack/core/ml/dataframe/evaluation/classification/MulticlassConfusionMatrix.java Outdated
Copy link
Member

@benwtrent benwtrent Oct 18, 2019

Choose a reason for hiding this comment

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

>= 0 check

Copy link
Contributor Author

@przemekwitek przemekwitek Oct 21, 2019

Choose a reason for hiding this comment

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

Done.

@przemekwitek przemekwitek force-pushed the revamp_multiclass_confusion_matrix branch 2 times, most recently from cc20654 to 9b1c359 Compare October 21, 2019 08:28
@przemekwitek
Copy link
Contributor Author

przemekwitek commented Oct 21, 2019

run elasticsearch-ci/packaging-sample-matrix

@przemekwitek przemekwitek force-pushed the revamp_multiclass_confusion_matrix branch from 9b1c359 to 92a6dd4 Compare October 21, 2019 09:36
@przemekwitek
Copy link
Contributor Author

przemekwitek commented Oct 21, 2019

run elasticsearch-ci/packaging-sample-matrix

@przemekwitek przemekwitek merged commit c9fea1e into elastic:master Oct 21, 2019
@przemekwitek przemekwitek deleted the revamp_multiclass_confusion_matrix branch October 21, 2019 12:36
@przemekwitek przemekwitek mentioned this pull request Oct 21, 2019
Merged
przemekwitek added a commit to przemekwitek/elasticsearch that referenced this pull request Oct 21, 2019
@przemekwitek
Change format of MulticlassConfusionMatrix result to be more self-exp…
4c9f6c6 
…lanatory (elastic#48174)
przemekwitek added a commit to przemekwitek/elasticsearch that referenced this pull request Oct 21, 2019
@przemekwitek
Change format of MulticlassConfusionMatrix result to be more self-exp…
784e064 
…lanatory (elastic#48174)
@przemekwitek przemekwitek mentioned this pull request Oct 21, 2019
Merged
przemekwitek added a commit that referenced this pull request Oct 21, 2019
@przemekwitek
[7.5] Change format of MulticlassConfusionMatrix result to be more se…
ed224d9 
…lf-explanatory (#48174) (#48295)
przemekwitek added a commit that referenced this pull request Oct 21, 2019
@przemekwitek
[7.x] Change format of MulticlassConfusionMatrix result to be more se…
2db2b94 
…lf-explanatory (#48174) (#48294)
This was referenced Feb 3, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benwtrent benwtrent benwtrent approved these changes

Assignees

No one assigned

Labels

>enhancement :ml Machine learning v7.5.0 v7.6.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@przemekwitek @elasticmachine @benwtrent @jakelandis