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

Remove feature possible values validation aside from type #44

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

lgvital
Copy link
Collaborator

@lgvital lgvital commented Apr 20, 2020

No description provided.

Copy link
Collaborator

@econti econti left a comment

Choose a reason for hiding this comment

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

Would validation on choices have to change as well (e.g. for decisions)

@@ -257,10 +257,6 @@ banditml.BanditAPI.prototype.validateAndFilterFeaturesInContext = function (cont
Array.isArray(possibleValues),
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

static/js/banditMl.js Show resolved Hide resolved
self.assert(
possibleValues.includes(context[featureName]),
`Value ${value} is not recognized among possible values for feature ${featureName}. Please update the possible values in Bandit ML.`
);
} else if (featureType === "P") {
self.assert(
Array.isArray(possibleValues),
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thats true, we'll probably remove this field completely. removing!

self.assert(
Array.isArray(possibleValues),
`Feature ${featureName} is a product set, but its possible values is not an array. Update the model appropriately in Bandit ML.`
);
self.assert(
typeof value === "string" || Array.isArray(value),
Copy link
Collaborator

Choose a reason for hiding this comment

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

not now, but worthwhile to validate string or array specifically of strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call, yes! ill add it in here since it's relatively trivial.

@lgvital lgvital merged commit 813f609 into master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants