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

improved test SmartTextMapVectorizerTest #296

Merged
merged 7 commits into from
Apr 30, 2019

Conversation

wsuchy
Copy link
Contributor

@wsuchy wsuchy commented Apr 18, 2019

Improved test by extending OpEstimatorSpec trait

@wsuchy wsuchy requested a review from tovbinm April 18, 2019 19:25
@codecov
Copy link

codecov bot commented Apr 18, 2019

Codecov Report

Merging #296 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
+ Coverage    86.4%   86.42%   +0.01%     
==========================================
  Files         322      322              
  Lines       10572    10572              
  Branches      349      566     +217     
==========================================
+ Hits         9135     9137       +2     
+ Misses       1437     1435       -2
Impacted Files Coverage Δ
...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 9333dcd...077d884. Read the comment docs.

* Estimator instance to be tested
*/
override val estimator: SmartTextMapVectorizer[TextMap] = new SmartTextMapVectorizer[TextMap]()
.setMaxCardinality(2).setNumFeatures(4).setMinSupport(1).setTopK(2).setPrependFeatureName(true)
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 perhaps test the stage with all the default settings, i.e. just val estimator: SmartTextMapVectorizer[TextMap] = new SmartTextMapVectorizer[TextMap]()

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wsuchy can you please address my comment so we can merge this?

@wsuchy wsuchy merged commit 6aba38e into master Apr 30, 2019
@Jauntbox Jauntbox mentioned this pull request May 2, 2019
@tovbinm tovbinm deleted the ks/improved-tests-SmartTextMapVectorizerTest branch May 8, 2019 21:16
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @wsuchy to sign the Salesforce.com Contributor License Agreement.

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