Skip to content
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.

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

[ML][Inference] Adding classification_weights to ensemble models #50874

Conversation

benwtrent
Copy link
Member

classification_weights are a way to allow models to
prefer specific classification results over others
this might be advantageous if classification value
probabilities are a known quantity and can improve
model error rates.

classification_weights are a way to allow models to
prefer specific classification results over others
this might be advantageous if classification value
probabilities are a known quantity and can improve
model error rates.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@benwtrent benwtrent marked this pull request as ready for review January 13, 2020 13:21
Copy link
Contributor

@tveasey tveasey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM. I noted some minor things and would like to see the classification labels in LangIdentNeuralNetwork::toXContent


if (classificationLabels != null && probabilities.size() != classificationLabels.size()) {
throw ExceptionsHelper
.serverError(
"model returned classification probabilities of size [{}] which is not equal to classification labels size [{}]",
"model returned classification probabilities of size [{}] which is not equal to classification labels size {}",
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better how it was before [{}]

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 possibly? I might want to switch this around a bit. It is writing out the classification labels array not the size. Woops!

Copy link
Member

Choose a reason for hiding this comment

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

Ha

@@ -156,11 +146,6 @@ public TargetType targetType() {
return TargetType.CLASSIFICATION;
}

@Override
public List<String> classificationLabels() {
return LANGUAGE_NAMES;
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding LANGUAGE_NAMES to toXContent and the parser. Tree includes the labels in its xcontent representation

/**
* @return List of featureNames expected by the model. In the order that they are expected
*/
List<String> getFeatureNames();
Copy link
Member

Choose a reason for hiding this comment

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

Both these methods are just not called anywhere? Is that why they are removed? If so 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I am cleaning up the interface. I added a bunch of stuff that I thought "all" models would need, and it turns out that that is not the case.

@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent merged commit f1eb3c3 into elastic:master Jan 14, 2020
@benwtrent benwtrent deleted the feature/ml-inference-add-classification-threshold branch January 14, 2020 16:32
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Jan 14, 2020
…stic#50874)

* [ML][Inference] Adding classification_weights to ensemble models

classification_weights are a way to allow models to
prefer specific classification results over others
this might be advantageous if classification value
probabilities are a known quantity and can improve
model error rates.
benwtrent added a commit that referenced this pull request Jan 14, 2020
) (#50994)

* [ML][Inference] Adding classification_weights to ensemble models

classification_weights are a way to allow models to
prefer specific classification results over others
this might be advantageous if classification value
probabilities are a known quantity and can improve
model error rates.
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
…stic#50874)

* [ML][Inference] Adding classification_weights to ensemble models

classification_weights are a way to allow models to
prefer specific classification results over others
this might be advantageous if classification value
probabilities are a known quantity and can improve
model error rates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants