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 PercentileCalibratorTest #318

Merged
merged 3 commits into from
May 13, 2019

Conversation

wsuchy
Copy link
Contributor

@wsuchy wsuchy commented May 13, 2019

No description provided.

Copy link
Collaborator

@tovbinm tovbinm left a comment

Choose a reason for hiding this comment

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

lgtm!

import spark.implicits._

Spec[PercentileCalibrator] should "return a minimum calibrated score of 0 and max of 99 when buckets is 100" in {
val testData = Seq(10, 100, 1000).map(_.toRealNN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use a bit of a larger sample?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already being tested by 1000 elements sample later in that test, the major purpose of those values and extending of OpEstimatorSpec trait is for checking de/serializing into json, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Base spec is supposed to test everything including transformations, serialization etc. Further tests usually test for additional behavior which is not covered in the base test. It makes sense to test as much as you can in the base test and avoid additional ones.

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   79.46%   86.51%   +7.05%     
==========================================
  Files         325      325              
  Lines       10581    10581              
  Branches      349      565     +216     
==========================================
+ Hits         8408     9154     +746     
+ Misses       2173     1427     -746
Impacted Files Coverage Δ
...ala/com/salesforce/op/features/types/package.scala 52.17% <0%> (+0.72%) ⬆️
...sforce/op/stages/impl/feature/Transmogrifier.scala 98.02% <0%> (+0.84%) ⬆️
...com/salesforce/op/features/types/FeatureType.scala 95.91% <0%> (+1.02%) ⬆️
...rce/op/stages/impl/preparators/SanityChecker.scala 91.59% <0%> (+1.08%) ⬆️
...scala/com/salesforce/op/features/FeatureLike.scala 42.39% <0%> (+1.08%) ⬆️
...op/evaluators/OpMultiClassificationEvaluator.scala 94.73% <0%> (+1.31%) ⬆️
...n/scala/com/salesforce/op/readers/DataReader.scala 95.23% <0%> (+1.58%) ⬆️
.../op/features/types/FeatureTypeSparkConverter.scala 99.11% <0%> (+1.76%) ⬆️
...ce/op/stages/impl/feature/OpOneHotVectorizer.scala 96.77% <0%> (+2.15%) ⬆️
.../scala/com/salesforce/op/dsl/RichTextFeature.scala 86.56% <0%> (+2.98%) ⬆️
... and 53 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 3992cfe...7d96dbd. Read the comment docs.

@wsuchy wsuchy merged commit d6bb99b into master May 13, 2019
@tovbinm tovbinm deleted the ks/improved-tests-PercentileCalibratorTest branch May 13, 2019 17:53
This was referenced Jul 10, 2019
@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