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

Property-based GLM test #427

Merged
merged 12 commits into from
Oct 24, 2019
Merged

Property-based GLM test #427

merged 12 commits into from
Oct 24, 2019

Conversation

michaelweilsalesforce
Copy link
Contributor

@michaelweilsalesforce michaelweilsalesforce commented Oct 18, 2019

Related issues
Adding more Tests for our ML capabilities (TDD)

Describe the proposed solution
Property based tests to check each of our the Regression Models (Linear Regression, Random Forest, GLM) will be picked based on the nature of the data (and response) generated

Describe alternatives you've considered
Doing the same tests on one single dataset instead

@tovbinm
Copy link
Collaborator

tovbinm commented Oct 19, 2019

@michaelweilsalesforce please update PR description. also, is it ready for review?

@@ -262,6 +261,7 @@ private[op] case object FitStagesUtil {
val stages = stagesLayer.map(_._1)
val (estimators, noFit) = stages.partition(_.isInstanceOf[Estimator[_]])
val fitEstimators = estimators.map { case e: Estimator[_] =>
println(e.getInputFeatures().toSeq)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove println

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.

great tests!

@michaelweilsalesforce
Copy link
Contributor Author

@Jauntbox These tests are actually painful for our runtime (~15-20 mins/ unit test)

@tovbinm
Copy link
Collaborator

tovbinm commented Oct 20, 2019

Builds will be killed if not output is produced for 10 minutes, so we can’t have an individual test take longer than that.

@codecov
Copy link

codecov bot commented Oct 20, 2019

Codecov Report

Merging #427 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #427      +/-   ##
==========================================
+ Coverage   86.94%   86.96%   +0.01%     
==========================================
  Files         337      337              
  Lines       11082    11083       +1     
  Branches      355      356       +1     
==========================================
+ Hits         9635     9638       +3     
+ Misses       1447     1445       -2
Impacted Files Coverage Δ
.../src/main/scala/com/salesforce/op/OpWorkflow.scala 88.88% <ø> (ø) ⬆️
...com/salesforce/op/utils/stages/FitStagesUtil.scala 94.8% <100%> (+0.06%) ⬆️
...es/src/main/scala/com/salesforce/op/OpParams.scala 85.71% <0%> (-4.09%) ⬇️
...in/scala/com/salesforce/op/cli/gen/AvroField.scala 76.92% <0%> (+2.56%) ⬆️
...cala/com/salesforce/op/cli/gen/ProblemSchema.scala 96.55% <0%> (+5.17%) ⬆️

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 b8bae1c...109fab1. Read the comment docs.

@michaelweilsalesforce michaelweilsalesforce merged commit 0f03c43 into master Oct 24, 2019
@tovbinm tovbinm deleted the mw/GLM-TDD branch November 26, 2019 05:10
@nicodv nicodv mentioned this pull request Jun 11, 2020
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

4 participants