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

fix local scoring with multipicklist features #243

Merged
merged 7 commits into from
Mar 20, 2019
Merged

Conversation

kinfaikan
Copy link
Contributor

Related issues
NA

Describe the proposed solution
Local scoring fails when there are multipicklist features because Seq can't be casted to MWrappedArray. Solution is to cast multipicklist value to Seq.

Describe alternatives you've considered
NA

Additional context
NA

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #243 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   86.39%   86.43%   +0.03%     
==========================================
  Files         312      312              
  Lines       10187    10187              
  Branches      336      564     +228     
==========================================
+ Hits         8801     8805       +4     
+ Misses       1386     1382       -4
Impacted Files Coverage Δ
.../op/features/types/FeatureTypeSparkConverter.scala 99.09% <100%> (+1.8%) ⬆️
...es/src/main/scala/com/salesforce/op/OpParams.scala 89.79% <0%> (+4.08%) ⬆️

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 27aab25...085bba6. Read the comment docs.

@@ -117,4 +120,47 @@ class OpWorkflowRunnerLocalTest extends FlatSpec with PassengerSparkFixtureTest
} score shouldBe expected
}

it should "handle multi picklist features without throwing an exception" in {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the tests seems an overkill for such a simple check ;) let's think if we can make is simpler and not so labor intensive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's simply test it in FeatureTypeSparkConverterTest

@@ -196,7 +196,7 @@ case object FeatureTypeSparkConverter {

// Sets
case wt if wt <:< weakTypeOf[t.MultiPickList] => (value: Any) =>
if (value == null) FeatureTypeDefaults.MultiPickList.value else value.asInstanceOf[MWrappedArray[String]].toSet
if (value == null) FeatureTypeDefaults.MultiPickList.value else value.asInstanceOf[Seq[String]].toSet
Copy link
Collaborator

Choose a reason for hiding this comment

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

you would also have to make the same fix for MultiPickListMap

@tovbinm tovbinm merged commit 9826b38 into master Mar 20, 2019
@tovbinm tovbinm deleted the kk/local-multipicklist branch March 20, 2019 02:27
@tovbinm tovbinm mentioned this pull request Apr 10, 2019
@salesforce-cla
Copy link

Thanks for the contribution! It looks like @kinfaikan is an internal user so signing the CLA is not required. However, we need to confirm this.

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.

None yet

2 participants