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

Implementation of trigonometric functions #331

Merged
merged 4 commits into from
Feb 26, 2019
Merged

Conversation

arbimo
Copy link
Contributor

@arbimo arbimo commented Feb 21, 2019

This is a proof of concept PR to gauge the interest in adding common trigonometric functions to Real.
It currently only contains a simple implementation for sin and cos to validate the approach and get early feedback before spending to much time on it.

If there is interest we should:

@avi-stripe
Copy link
Contributor

Interesting. Can you say something about the use cases here?

@arbimo
Copy link
Contributor Author

arbimo commented Feb 22, 2019

Essentially, I would like to use rainier-core's features of online compilation and automated differentiation for numeric optimization. My current use case is in the control of hybrid systems where trigonometric functions are ubiquitous. Since such trigonometric functions can not be expressed in terms of anything else their absence in Rainier means I cannot use it and would need to maintain a fork.

Overall, implementations of trigo functions should not be intrusive: its mostly additional unary ops as the ones for sin and cos. With enough automated test the risks would be very low as well.

Another option would be to add an extension point to allow user-defined operations (which could also help for #227) but this would require some non-trivial refactoring and I really believe trigonometric functions have their place in the core.

@avi-stripe
Copy link
Contributor

Got it, thanks for the context. I'm +1 to continuing with these. Thanks so much for the contributions!

@avi-stripe
Copy link
Contributor

As a side note - I gather from this that you would probably be using the compute and ir packages but not the core package with RandomVariable and Distribution? If at some point it makes sense to split those out into a separate module I'm sure we can do that.

@arbimo
Copy link
Contributor Author

arbimo commented Feb 23, 2019

Great, I'll start working on it.

Indeed, I am only interested in compute and ir packages. It might make sense to move them to their own module since they are very general purpose and might be useful in a lot of different contexts. However, as long as the rest of rainier-core remains small and with no additional dependencies it is more of a marketing decision with no important technical implications.

@arbimo arbimo marked this pull request as ready for review February 26, 2019 18:23
@codecov-io
Copy link

codecov-io commented Feb 26, 2019

Codecov Report

Merging #331 into develop will increase coverage by 0.39%.
The diff coverage is 65.67%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #331      +/-   ##
===========================================
+ Coverage    47.89%   48.29%   +0.39%     
===========================================
  Files           80       80              
  Lines         2758     2812      +54     
  Branches       214      217       +3     
===========================================
+ Hits          1321     1358      +37     
- Misses        1437     1454      +17
Impacted Files Coverage Δ
...ore/src/main/scala/com/stripe/rainier/ir/Ops.scala 100% <ø> (ø) ⬆️
...e/src/main/scala/com/stripe/rainier/ir/IRViz.scala 0% <0%> (ø) ⬆️
.../scala/com/stripe/rainier/ir/MethodGenerator.scala 81.6% <100%> (+1.36%) ⬆️
...c/main/scala/com/stripe/rainier/compute/Real.scala 75% <100%> (+6.25%) ⬆️
...ain/scala/com/stripe/rainier/compute/RealOps.scala 62.06% <55.55%> (-3.15%) ⬇️
...in/scala/com/stripe/rainier/compute/Gradient.scala 93.44% <85.71%> (+0.71%) ⬆️
.../scala/com/stripe/rainier/compute/LogLineOps.scala 100% <0%> (+3.44%) ⬆️

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 3026eae...d149ffa. Read the comment docs.

@arbimo
Copy link
Contributor Author

arbimo commented Feb 26, 2019

@avi-stripe This should be ready for review now.
In the end, there are only intrinsic for (a)sin, (a)cos, (a)tan. Of those only tan could have been defined in term of sin and cos.

I have also added methods on Real for hyperbolic functions that are defined in terms of the existing primitives. Those are not essential to this PR as they only introduce syntactic sugar.

@avi-stripe
Copy link
Contributor

Wonderful, thank you!

@avi-stripe avi-stripe merged commit 673408e into stripe:develop Feb 26, 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