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

Add multiclassification topk and confmatrix metrics to model insights serialization format #537

Merged
merged 5 commits into from
Jan 15, 2021

Conversation

feifjiang
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #537 (9ea7e44) into master (37df638) will increase coverage by 7.97%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
+ Coverage   78.80%   86.78%   +7.97%     
==========================================
  Files         347      347              
  Lines       12019    12025       +6     
  Branches      394      384      -10     
==========================================
+ Hits         9472    10436     +964     
+ Misses       2547     1589     -958     
Impacted Files Coverage Δ
...c/main/scala/com/salesforce/op/ModelInsights.scala 90.12% <ø> (ø)
...op/evaluators/OpMultiClassificationEvaluator.scala 95.78% <100.00%> (+3.28%) ⬆️
...s/impl/preparators/DerivedFeatureFilterUtils.scala 93.08% <0.00%> (+0.62%) ⬆️
...com/salesforce/op/features/FeatureSparkTypes.scala 99.14% <0.00%> (+0.85%) ⬆️
.../op/features/types/FeatureTypeSparkConverter.scala 99.12% <0.00%> (+0.87%) ⬆️
...com/salesforce/op/utils/stages/FitStagesUtil.scala 93.42% <0.00%> (+1.31%) ⬆️
.../main/scala/com/salesforce/op/OpWorkflowCore.scala 95.45% <0.00%> (+1.51%) ⬆️
...sforce/op/stages/impl/feature/Transmogrifier.scala 98.05% <0.00%> (+1.94%) ⬆️
...orce/op/stages/impl/tuning/OpCrossValidation.scala 97.95% <0.00%> (+2.04%) ⬆️
...op/stages/impl/selector/ModelSelectorSummary.scala 91.83% <0.00%> (+2.04%) ⬆️
... and 70 more

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 37df638...9ea7e44. Read the comment docs.

@@ -232,8 +232,8 @@ class RecordInsightsLOCOTest extends FunSpec with TestSparkContext with RecordIn
info("Each feature vector should only have either three or four non-zero entries. One each from country and " +
"picklist, while currency can have either two (if it's null the currency column will be filled with the mean)" +
" or just one if it's not null.")
it("should pick between 1 and 4 of the features") {
all(parsed.map(_.size)) should (be >= 1 and be <= 4)
it("should pick between 0 and 4 of the features") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tovbinm this test is quite flaky. In one of the failed runs, the value was 0 and not between 1 and 4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leahmcguire @Jauntbox do you have any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's still an open PR to address this: #423

We could use a better test, but for now we can start by getting rid of the flakiness.

feifjiang and others added 2 commits January 7, 2021 20:06
…est to check serialization/deseriazalition of multiclassification metrics
@winterslu
Copy link
Contributor

@crupley @nicodv include someone from the team to prioritize review this PR, changes in this PR will fix test that's currently blocking https://github.salesforceiq.com/einstein/einstein1/pull/4010 for bumping version in automl

Copy link
Contributor

@nicodv nicodv left a comment

Choose a reason for hiding this comment

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

LGTM

@TuanNguyen27 TuanNguyen27 merged commit ec50a92 into salesforce:master Jan 15, 2021
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