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

[AiLab] correctly access data in testValues #39097

Merged
merged 2 commits into from
Feb 18, 2021
Merged

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Feb 17, 2021

tldr: "Whipped Cream" != "WhippedCream"

We got reports from the curriculum team of a bug where the prediction in AI Lab didn't match the prediction in App Lab for the same trained model with the same test values. This initially seemed very worrisome, but was actually quite a straightforward fix.

In #39067 we striped column names from AI Lab trained models of whitespace and special characters so they could be used as design element ids and keys in the testValues object.

However, in the predict function we were not correctly accessing values in testValues to pass to the convertTestValue function from #39044. Therefore, certain features (particularly those with spaces, e.g. "whipped cream" and "fried chicken") were being converted to NaN, which was causing the trained model to return inaccurate predictions.

BEFORE:
Screen Shot 2021-02-16 at 10 23 31 PM

AFTER:
Screen Shot 2021-02-16 at 10 06 41 PM

Now, predictions are consistent across AI Lab and App Lab.

@@ -48,7 +48,7 @@ export function predict(modelData) {
convertTestValue(
modelData.featureNumberKey,
feature,
modelData.testData[feature]
modelData.testData[feature.replace(/\W/g, '')]
Copy link
Member

Choose a reason for hiding this comment

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

Since we have this snippet a few times now, can we hoist it to a helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where should it live? It's used in both ai.js and here in MLTrainers.js. Just pick one and export to the other?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too sure about the /apps heirarchy, but I think it makes sense for applab/ai.js to reach into its parent directory for such a utility function (since it seems we have a variety of "utils" and "common" files there). It might be cleanest to add this to apps/src/utils.js so that we don't create a dependency between the two existing source files, or to create a new apps/src/aiUtils.js file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I went with the aiUtils option.

@Erin007 Erin007 merged commit 70e6767 into staging Feb 18, 2021
@Erin007 Erin007 deleted the predict-inconsistencies branch February 18, 2021 00:57
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.

None yet

2 participants