DataCutter-related fixes for multiclass by gerashegalov · Pull Request #263 · salesforce/TransmogrifAI

gerashegalov · 2019-04-03T10:45:08Z

Related issues
Fixes #245

label sort by count is nondeterministic for labels with the same count
data cutter label trimming in validation is not called prior to bestEstimator
column metadata is not updated after trimming
in DataCutter.estimate whereas filter before sort is good for performance it may not drop labels non-deterministically unlike filtering over a stable order.

Describe the proposed solution
Remove non-determinism

codecov · 2019-04-03T18:16:47Z

Codecov Report

Merging #263 into master will decrease coverage by 1.85%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
- Coverage   86.61%   84.75%   -1.86%     
==========================================
  Files         315      315              
  Lines       10345    10369      +24     
  Branches      346      563     +217     
==========================================
- Hits         8960     8788     -172     
- Misses       1385     1581     +196

Impacted Files	Coverage Δ
.../stages/impl/feature/OpStringIndexerNoFilter.scala	`100% <100%> (ø)`	⬆️
...sforce/op/stages/impl/selector/ModelSelector.scala	`98.18% <100%> (+0.03%)`	⬆️
.../salesforce/op/stages/impl/tuning/DataCutter.scala	`91.54% <90%> (-4.11%)`	⬇️
...sforce/op/stages/base/binary/BinaryEstimator.scala	`0% <0%> (-100%)`	⬇️
...stages/base/sequence/BinarySequenceEstimator.scala	`0% <0%> (-100%)`	⬇️
...orce/op/stages/base/ternary/TernaryEstimator.scala	`0% <0%> (-100%)`	⬇️
...m/salesforce/op/utils/spark/OpVectorMetadata.scala	`0% <0%> (-85.46%)`	⬇️
...sforce/op/utils/spark/OpVectorColumnMetadata.scala	`0% <0%> (-76.09%)`	⬇️
...m/salesforce/op/aggregators/ExtendedMultiset.scala	`0% <0%> (-75%)`	⬇️
...ages/impl/classification/OpXGBoostClassifier.scala	`6.89% <0%> (-25.87%)`	⬇️
... and 11 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 6bb63ba...8333743. Read the comment docs.

codecov · 2019-04-03T18:16:47Z

Codecov Report

Merging #263 into master will decrease coverage by 3.77%.
The diff coverage is 99.02%.

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
- Coverage   86.24%   82.47%   -3.78%     
==========================================
  Files         320      321       +1     
  Lines       10471    10539      +68     
  Branches      352      558     +206     
==========================================
- Hits         9031     8692     -339     
- Misses       1440     1847     +407

Impacted Files	Coverage Δ
...sforce/op/stages/impl/selector/ModelSelector.scala	`98.24% <100%> (+0.09%)`	⬆️
...org/apache/spark/ml/attribute/MetadataHelper.scala	`100% <100%> (ø)`
.../stages/impl/feature/OpStringIndexerNoFilter.scala	`100% <100%> (ø)`	⬆️
...alesforce/op/stages/impl/tuning/DataBalancer.scala	`96.29% <100%> (+0.1%)`	⬆️
...om/salesforce/op/stages/impl/tuning/Splitter.scala	`97.72% <100%> (+0.58%)`	⬆️
...e/op/stages/impl/selector/ModelSelectorNames.scala	`100% <100%> (ø)`	⬆️
...alesforce/op/stages/impl/tuning/DataSplitter.scala	`66.66% <100%> (ø)`	⬆️
.../salesforce/op/stages/impl/tuning/DataCutter.scala	`96.84% <98.59%> (+1.18%)`	⬆️
...alesforce/op/cli/gen/templates/SimpleProject.scala	`0% <0%> (-100%)`	⬇️
.../scala/com/salesforce/op/cli/gen/ProblemKind.scala	`0% <0%> (-100%)`	⬇️
... and 12 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 9357a89...3d4198b. Read the comment docs.

tovbinm · 2019-04-03T18:17:19Z

@gerashegalov can you elaborate more on the proposed fixes? thanks.

gerashegalov · 2019-04-03T19:58:01Z

@tovbinm there are multiple issues.

One is the sort stability when there are multiple top labels with identical cardinality. We can solve this by adding the label as a secondary sort key. Similar issue with filtering labels for the minFraction prior to sorting. And #251 caused the data cutter omission to begin with

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala

core/src/main/scala/com/salesforce/op/stages/impl/selector/ModelSelector.scala

tovbinm · 2019-04-04T04:24:52Z

@gerashegalov please let us know when you want the PR reviewed.

gerashegalov · 2019-04-04T17:56:15Z

@tovbinm it's ready

core/src/main/scala/com/salesforce/op/stages/impl/selector/ModelSelector.scala

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala

tovbinm

Can you please add a new test case explicitly for label trimming issue?

gerashegalov · 2019-04-05T13:57:38Z

not ready for review yet, checkpointing WIP, sorting out test issues.

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala

gerashegalov · 2019-04-15T21:24:41Z

marking as ready for review since all unit testing are passing

leahmcguire

Just remove the parts that were in there for testing and LGTM

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala

test table num_vals

… for test

leahmcguire

LGTM!!!!

tovbinm · 2019-04-19T19:15:58Z

🎆 🍾

gerashegalov requested review from leahmcguire and tovbinm April 3, 2019 10:45

salesforce-cla bot added the cla:signed label Apr 3, 2019

gerashegalov force-pushed the gera/factorlabeltrim branch from 8333743 to 618bd92 Compare April 3, 2019 19:38

leahmcguire reviewed Apr 3, 2019

View reviewed changes

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala Outdated Show resolved Hide resolved

core/src/main/scala/com/salesforce/op/stages/impl/selector/ModelSelector.scala Outdated Show resolved Hide resolved

gerashegalov force-pushed the gera/factorlabeltrim branch from 6abd993 to 7571463 Compare April 4, 2019 17:54

gerashegalov added the ready for review label Apr 4, 2019

tovbinm reviewed Apr 5, 2019

View reviewed changes

gerashegalov added work in progress and removed ready for review labels Apr 5, 2019

gerashegalov force-pushed the gera/factorlabeltrim branch 2 times, most recently from 843f8cd to a43409a Compare April 9, 2019 12:16

leahmcguire reviewed Apr 9, 2019

View reviewed changes

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala Outdated Show resolved Hide resolved

leahmcguire reviewed Apr 10, 2019

View reviewed changes

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala Outdated Show resolved Hide resolved

leahmcguire reviewed Apr 10, 2019

View reviewed changes

gerashegalov force-pushed the gera/factorlabeltrim branch 3 times, most recently from b7c72de to 5a5b19d Compare April 15, 2019 07:13

gerashegalov added ready for review and removed work in progress labels Apr 15, 2019

leahmcguire reviewed Apr 16, 2019

View reviewed changes

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala Outdated Show resolved Hide resolved

core/src/main/scala/com/salesforce/op/stages/impl/tuning/DataCutter.scala Outdated Show resolved Hide resolved

gerashegalov added 21 commits April 18, 2019 12:36

if inverted

812e9b4

LR repro'ed

d44e477

WIP

448ae1f

test vectors

0f24e4f

test both indexed and non-idexed data with data cutter

f4286a8

Leah's comments

b908673

test table num_vals

param for number dropped lables to log

ce679ff

WIP

ccb1cfa

WIP

bcbfe06

WIP

c8a202f

WIP

881f7cc

formatting

e05dda6

Replace cacheValidatedDFForTesting with the extended DataCutter class…

fff626b

… for test

mt feedback WIP

6c5631a

build failure fix

d296b0c

whitespace

fff650b

column index constants

2f8ab2e

Param rename

e5ff482

CodeFactor issue

0b07a3d

upper bound to 1000

3c614fc

Logging fix

17c53fb

gerashegalov force-pushed the gera/factorlabeltrim branch from 27992db to 17c53fb Compare April 18, 2019 23:59

Merge branch 'master' into gera/factorlabeltrim

3d4198b

leahmcguire approved these changes Apr 19, 2019

View reviewed changes

tovbinm approved these changes Apr 19, 2019

View reviewed changes

gerashegalov merged commit 213962e into salesforce:master Apr 19, 2019

gerashegalov deleted the gera/factorlabeltrim branch April 19, 2019 18:49

Jauntbox mentioned this pull request May 2, 2019

Release 0.5.3 #305

Merged

michaelweilsalesforce mentioned this pull request Jul 10, 2019

0.6.0 Release #360

Closed

Conversation

gerashegalov commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 3, 2019

Codecov Report

Uh oh!

codecov bot commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tovbinm commented Apr 3, 2019

Uh oh!

gerashegalov commented Apr 3, 2019

Uh oh!

Uh oh!

Uh oh!

tovbinm commented Apr 4, 2019

Uh oh!

gerashegalov commented Apr 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tovbinm left a comment

Choose a reason for hiding this comment

Uh oh!

gerashegalov commented Apr 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gerashegalov commented Apr 15, 2019

Uh oh!

leahmcguire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leahmcguire left a comment

Choose a reason for hiding this comment

Uh oh!

tovbinm commented Apr 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gerashegalov commented Apr 3, 2019 •

edited

Loading

codecov bot commented Apr 3, 2019 •

edited

Loading