-
Notifications
You must be signed in to change notification settings - Fork 394
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
ignoring cardinality in modelinsights serialization #474
Conversation
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Winters Lu <k***@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. |
Codecov Report
@@ Coverage Diff @@
## master #474 +/- ##
=======================================
Coverage 86.99% 86.99%
=======================================
Files 345 345
Lines 11622 11624 +2
Branches 392 386 -6
=======================================
+ Hits 10110 10112 +2
Misses 1512 1512
Continue to review full report at Codecov.
|
@@ -968,4 +969,6 @@ class ModelInsightsTest extends FlatSpec with PassengerSparkFixtureTest with Dou | |||
"second" -> classOf[SingleMetric] | |||
) | |||
} | |||
|
|||
it should "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it?
@gerashegalov this is fixing the final missing piece of very old bug, need to get this merged so we can move forward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -456,6 +456,7 @@ class ModelInsightsTest extends FlatSpec with PassengerSparkFixtureTest with Dou | |||
i.featureName shouldEqual o.featureName | |||
i.featureType shouldEqual o.featureType | |||
i.derivedFeatures.zip(o.derivedFeatures).foreach { case (ii, io) => ii.corr shouldEqual io.corr } | |||
o.distributions.foreach { o => o.cardEstimate shouldEqual None} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test could be more obvious asserting that the pre-serialization object has cardEstimate. And that it loses cardEstimates per requirement after serialization. As opposed to just checking the post-serialization state.
However, I verified that the test fails without your main change, so we should be ok to merge it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gerashegalov I updated the counter part of this test case
Related issues
Tokens of text field are printing out while internal usage.
Describe the proposed solution
Ignore cardinality field in modelInsights serialization, addition to featureDistribution
Additional context
Previous PR #420 & #438 & #447