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

Math transformers part 2 of 2 #257

Merged
merged 20 commits into from
Apr 4, 2019
Merged

Math transformers part 2 of 2 #257

merged 20 commits into from
Apr 4, 2019

Conversation

leahmcguire
Copy link
Collaborator

Related issues
Made transformers and shortcuts for common math operations

@codecov
Copy link

codecov bot commented Apr 1, 2019

Codecov Report

Merging #257 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   86.61%   86.63%   +0.02%     
==========================================
  Files         315      315              
  Lines       10345    10366      +21     
  Branches      346      546     +200     
==========================================
+ Hits         8960     8981      +21     
  Misses       1385     1385
Impacted Files Coverage Δ
...orce/op/stages/impl/feature/MathTransformers.scala 100% <100%> (ø) ⬆️
...ala/com/salesforce/op/dsl/RichNumericFeature.scala 100% <100%> (ø) ⬆️

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 6bb63ba...55df28d. Read the comment docs.

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

In your tests you can also use RandomReal, RandomIntegral, RandomBinary etc. from out test kit to generate test samples instead if hard coded values.

uid: String = UID[FloorTransformer[_]]
)(
implicit override val tti: TypeTag[I]
) extends UnaryTransformer[I, Integral](operationName = "floor", 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.

operationName = "round"

f2.originStage.isInstanceOf[AbsoluteValueTransformer[_]] shouldBe true
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove redundant trailing endlines in all test files

val sample = Seq(Real(-1.3), Real(-4.9), Real.empty, Real(5.1), Real(-5.1), Real(0.1), Real(2.5), Real(0.4))
val (inputData, f1) = TestFeatureBuilder(sample)
val transformer: ExpTransformer[Real] = new ExpTransformer[Real]().setInput(f1)
override val expectedResult: Seq[Real] = Seq(Real(math.exp(-1.3)), Real(math.exp(-4.9)), Real.empty,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seq(..).map(math.exp).map(_.toReal)

val (inputData, f1) = TestFeatureBuilder(sample)
val transformer: PowerTransformer[Real, Double] = new PowerTransformer[Real, Double](3.0).setInput(f1)
override val expectedResult: Seq[Real] = Seq(Real(math.pow(-1.3, 3)), Real(math.pow(-4.9, 3)), Real.empty,
Real(math.pow(5.1, 3)), Real(math.pow(-5.1, 3)), Real(math.pow(0.1, 3)), Real(math.pow(2.5, 3)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here Seq(...).map(math.pow).map(_.toReal)

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!!

@tovbinm tovbinm merged commit 209647c into master Apr 4, 2019
@tovbinm tovbinm deleted the lm/mathTransformers2 branch April 4, 2019 03:56
@tovbinm
Copy link
Collaborator

tovbinm commented Apr 4, 2019

@leahmcguire one thought I had after I merged the PR is that for features of more concrete numeric types, i.e. RealNN, Currency etc, all these shortcuts would "downgrade" them into their base type of Real, which might not be desired. Perhaps we should make these transformers to be more specific so that the output type would match the input type, i.e:

class ExpTransformer[I <: Real]
(
  uid: String = UID[ExpTransformer[_]]
)(
  implicit override val tti: TypeTag[I]
) extends UnaryTransformer[I, I](operationName = "exp", uid = uid){
  private val convert = FeatureTypeSparkConverter[I]()
  override def transformFn: I => I = (i: I) => convert.fromSpark(i.toDouble.map(math.exp).filter(Number.isValid).orNull)
}

@leahmcguire
Copy link
Collaborator Author

Well I'm not sure that the exp (or log or power) of currency is still currency. I'm sure percent is not still a percent. The RealNN is the only one that it may not be reasonable to downgrade and the difference in treatment between the Real and RealNN is just the null check and fill - which would have no effect if there are no nulls...

@salesforce-cla
Copy link

Thanks for the contribution! Unfortunately we can't verify the commit author(s): leahmcguire <l***@s***.com> Leah McGuire <l***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

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