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

Part 1 of 2 - Making concrete math transformers #255

Merged
merged 10 commits into from
Mar 29, 2019

Conversation

leahmcguire
Copy link
Collaborator

Related issues
Want to use concrete math transformer classes so they are easier to generate outside of the shortcuts

Describe the proposed solution
Move from lamda definitions of transformers to defined classes

Additional context
Next PR will add more math transformers

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, see some comments.

uid: String = UID[ScalarAddTransformer[_, _]]
)(
implicit override val tti: TypeTag[I],
n: Numeric[N]
Copy link
Collaborator

Choose a reason for hiding this comment

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

val n: Numeric[N] (here and in other transformers)

)(
implicit override val tti1: TypeTag[I1],
override val tti2: TypeTag[I2]
) extends BinaryTransformer[I1, I2, Real](operationName = "addition", uid = uid){
Copy link
Collaborator

Choose a reason for hiding this comment

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

the operation name was "plus" is there a reason to change it (same with other transformers). I would rather keep it the same as before.

@@ -134,15 +134,55 @@ class RichNumericFeatureTest extends FlatSpec with FeatureTestBase with RichNume
// TODO: add vectorize() test
}

Spec[RichRealNNFeature] should "have tests" in {
// TODO: add tests
Spec[RichRealNNFeature] should "perform math functions correctly" in {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the tests!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an easy way to use our OpTransformerSpec here, or would we have to make a separate test class for each transformer we're trying to test?

n: Numeric[N]
) extends UnaryTransformer[I, Real](operationName = "scalarDivide", uid = uid){
override def transformFn: I => Real = (i: I) => i.toDouble.map(_ / n.toDouble(scalar)).filter(Number.isValid).toReal
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might need an endline here

@tovbinm
Copy link
Collaborator

tovbinm commented Mar 28, 2019

@leahmcguire can you also please add a test for each of the transformers using the transformer test spec?

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@32ec731). Click here to learn what that means.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #255   +/-   ##
=========================================
  Coverage          ?   80.94%           
=========================================
  Files             ?      315           
  Lines             ?    10282           
  Branches          ?      556           
=========================================
  Hits              ?     8323           
  Misses            ?     1959           
  Partials          ?        0
Impacted Files Coverage Δ
...ala/com/salesforce/op/dsl/RichNumericFeature.scala 84.78% <50%> (ø)
...orce/op/stages/impl/feature/MathTransformers.scala 50% <50%> (ø)

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 32ec731...f0679af. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #255 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   86.57%   86.58%   +<.01%     
==========================================
  Files         314      315       +1     
  Lines       10302    10286      -16     
  Branches      335      560     +225     
==========================================
- Hits         8919     8906      -13     
+ Misses       1383     1380       -3
Impacted Files Coverage Δ
...ala/com/salesforce/op/dsl/RichNumericFeature.scala 100% <100%> (ø) ⬆️
...orce/op/stages/impl/feature/MathTransformers.scala 100% <100%> (ø)
.../op/features/types/FeatureTypeSparkConverter.scala 99.09% <0%> (+0.9%) ⬆️
...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 32ec731...6fc525f. Read the comment docs.

@leahmcguire leahmcguire merged commit 9789468 into master Mar 29, 2019
@leahmcguire leahmcguire deleted the lm/mathTransformers branch March 29, 2019 02:26
@tovbinm tovbinm mentioned this pull request Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants