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

fixed model insights exception when features are excluded from sanity… #147

Merged
merged 7 commits into from
Oct 4, 2018

Conversation

leahmcguire
Copy link
Collaborator

… checker correlation calculations

Related issues
Model insights used to throw runtime exception if a feature was not in the sanity checker correlation computation. However some features can be excluded from these calculations for performance so this was a problem.

Describe the proposed solution
If the feature is not found in the sanity checker calculations a None is returned for correlation.

@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

Merging #147 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   86.38%   86.36%   -0.02%     
==========================================
  Files         299      299              
  Lines        9752     9750       -2     
  Branches      343      550     +207     
==========================================
- Hits         8424     8421       -3     
- Misses       1328     1329       +1
Impacted Files Coverage Δ
...c/main/scala/com/salesforce/op/ModelInsights.scala 93.33% <100%> (+0.77%) ⬆️
.../salesforce/op/features/FeatureBuilderMacros.scala 0% <0%> (-100%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b4eef...7f18ea1. Read the comment docs.

@@ -152,6 +153,22 @@ class ModelInsightsTest extends FlatSpec with PassengerSparkFixtureTest {
insights.stageInfo.keys.size shouldEqual 8
}

it should "return model insights even when correlation is turned off for some features" in {
val featuresFinal = Seq(description.vectorize(10, false, 1, true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please name the arguments to vectorize calls.

Copy link
Collaborator

@tovbinm tovbinm left a comment

Choose a reason for hiding this comment

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

one minor comment, otherwise lgtm

@leahmcguire leahmcguire merged commit 01f6e0f into master Oct 4, 2018
@leahmcguire leahmcguire deleted the lm/insightsFail branch October 4, 2018 23:28
@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): leahmcguire <l***@s***.com> Leah McGuire <l***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

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.

2 participants