NotificationsYou must be signed in to change notification settings
Fork401
Star2.3k

loco test bug fix#423

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

AdamChit wants to merge15 commits intosalesforce:master

base:master

Choose a base branch

fromAdamChit:achit/LOCO-Test-Bug-Fix

Open

loco test bug fix#423

AdamChit wants to merge15 commits intosalesforce:masterfromAdamChit:achit/LOCO-Test-Bug-Fix

Conversation

Copy link

Collaborator

AdamChit commentedOct 16, 2019

Related issues
test refactor made in#412 causes one of the test cases to fail.

Specially this test assumed that at-least 1 of the 4 features would be selectedhttps://github.com/salesforce/TransmogrifAI/blob/master/core/src/test/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCOTest.scala#L236

Describe the proposed solution
Test should allow the case where none of the features are picked

Additional context
example of the test failing:

AdamChitand others added11 commits

September 27, 2019 16:11

Changes to FunSpec and refactored test

a17c13c

refactored test to make it more readable and changed to Funspec

b0cdfae

Merge branch 'master' into LocoTestRefactor

ebedf1d

Changes to FunSpec and refactored test

96ec077

refactored test to make it more readable and changed to Funspec

b3ca610

Merge branch 'LocoTestRefactor' ofhttps://github.com/AdamChit/Transm…

fb28e65

…ogrifAI into LocoTestRefactormerge master

revert formatting to follow scala style

3095e19

more descriptive title for section

973aa0d

threshold was too large and would fail on some runs of the test

e31bd22

fork sync

acd97a6

non of the features could be selected

236bb7c

AdamChit requested review fromJauntbox,gerashegalov,leahmcguire,tovbinm andwsuchy ascode owners

October 16, 2019 16:45

salesforce-clabot added the cla:signed label

Oct 16, 2019

AdamChit requested a review fromcrupley

October 16, 2019 16:46

Copy link

codecovbot commentedOct 16, 2019•
edited
Loading

Codecov Report

Merging#423 (b4a2116) intomaster (ac83ad7) willdecrease coverage by60.42%.
The diff coverage isn/a.

@@             Coverage Diff             @@##           master     #423       +/-   ##===========================================- Coverage   86.94%   26.52%   -60.43%===========================================  Files         340      337        -3       Lines       11388    11131      -257       Branches      363      593      +230     ===========================================- Hits         9901     2952     -6949- Misses       1487     8179     +6692

Impacted Files	Coverage Δ
...main/scala/com/salesforce/op/dsl/RichFeature.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...main/scala/com/salesforce/op/filters/Summary.scala	`0.00% <0.00%> (-100.00%)`	⬇️
.../scala/com/salesforce/op/cli/gen/ProblemKind.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...n/scala/com/salesforce/op/dsl/RichSetFeature.scala	`0.00% <0.00%> (-100.00%)`	⬇️
.../scala/com/salesforce/op/dsl/RichListFeature.scala	`0.00% <0.00%> (-100.00%)`	⬇️
.../scala/com/salesforce/op/stages/impl/package.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...cala/com/salesforce/op/cli/gen/FileInProject.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...scala/org/apache/spark/util/SparkThreadUtils.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...cala/com/salesforce/op/OpWorkflowModelWriter.scala	`0.00% <0.00%> (-100.00%)`	⬇️
...cala/com/salesforce/op/evaluators/Evaluators.scala	`0.00% <0.00%> (-100.00%)`	⬇️
... and201 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updateac83ad7...b6829fe. Read thecomment docs.

tovbinm reviewed

Oct 16, 2019

View reviewed changes

Copy link

Collaborator

tovbinm left a comment

Choose a reason for hiding this comment

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

Why would it not pick any features?

leahmcguire reviewed

Oct 17, 2019

View reviewed changes

core/src/test/scala/com/salesforce/op/stages/impl/insights/RecordInsightsLOCOTest.scala

		it("should pick between1 and 4 of the features") {
		all(parsed.map(_.size)) should (be>=1 and be<=4)
		it("should pick between0 and 4 features") {
		all(parsed.map(_.size)) should (be>=0 and be<=4)

Copy link

Collaborator

leahmcguireOct 17, 2019

Choose a reason for hiding this comment

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

this seems like it is not checking very much... can you do a check for the conditions described above and then do the size check?

Copy link

CollaboratorAuthor

AdamChit commentedOct 19, 2019

Why would it not pick any features?

So in the test the labels are generated based on the PickList feature:

        p.value match {          case Some("A") | Some("B") | Some("C") => RealNN(1.0)          case _ => RealNN(0.0)        }      )

And all the features have a chance of being empty:

      val countryData: Seq[Country] = RandomText.countries.withProbabilityOfEmpty(0.3).take(numRows).toList      val pickListData: Seq[PickList] = RandomText.pickLists(domain = List("A", "B", "C", "D", "E", "F", "G"))        .withProbabilityOfEmpty(0.1).limit(numRows)      val currencyData: Seq[Currency] = RandomReal.logNormal[Currency](mean = 10.0, sigma = 1.0)        .withProbabilityOfEmpty(0.3).limit(numRows)

One case that could happen:
2) All the feature are set to empty
3) LOCO changes the pickList from empty to non-empty and because it is not in the set {A,B,C} it isNOT a strong of a predictor
=> label wouldn't change => pickList would not have a strong insight => No feature is selected

Note: the chance of this happening is very low which explains why it doesn't always appear in the CI build.