Implements inverse hyperbolic operations (fix for #7531) #10598

lakshayg · 2017-06-09T15:00:46Z

tensorflow-jenkins · 2017-06-09T15:00:48Z

Can one of the admins verify this patch?

caisq · 2017-06-11T18:13:40Z

lakshayg · 2017-06-12T05:57:46Z

I ran the tests which failed here on my machine and found that the only cases which were failing are the ones in which tf.acosh evaluates to inf/-inf and np.arccosh evaluates to nan/-nan. ~~Should I change the test cases or do something else? Any ideas on how to proceed?~~ I have changed the test cases and also fixed a bug in test functions for asinh and acosh. The failed tests are passing now.

rmlarsen · 2017-06-14T00:44:14Z

@tensorflow-jenkins test this please

lakshayg · 2017-06-14T03:20:33Z

The failures in tests/build seem unrelated to the changes made by this pull request.

martinwicke · 2017-06-14T03:57:21Z

Jenkins, test this please.

martinwicke · 2017-06-14T05:37:31Z

The cwise ops test failure (only on Windows, ugh) is probably caused by this CL -- the tolerances appear to be too tight.

lakshayg · 2017-06-14T06:00:19Z

Any hints on how to proceed?

martinwicke · 2017-06-14T06:06:51Z

I would look at which test fails, exactly, and adjust the tolerance for that test in the test code. I think the values are still close so this is not a real problem, just numerics and a particularly unfriendly function.

lakshayg · 2017-06-14T06:11:50Z

If that is the case then this PR would have to wait for a few days since I do not have access to my machine and would be able to fix it only after the 16th. BTW since I do not have a windows machine, how can I identify the function which is failing?

martinwicke · 2017-06-14T06:24:31Z

No problem.

martinwicke · 2017-06-16T18:26:43Z

This test failure is not due to this PR. We'll review API changes on Monday, should be able to merge then.

lakshayg · 2017-06-17T01:15:00Z

Great, thanks!

lakshayg · 2017-06-17T07:26:33Z

Since the tests for acosh and asinh were passing, I have also added a commit which implements atanh thus completely resolving #7531. Please review.

rmlarsen · 2017-06-20T20:53:17Z

@lakshayg that should give 1/0 = inf. I'm not sure why it produces NaN instead of inf, but that is unrelated to your change.

lakshayg · 2017-06-21T03:20:32Z

@rmlarsen I just compiled the binary and checked what it gave for x=1 and the answer is inf. I guess that everything is working fine and this PR can be merged.

rmlarsen · 2017-06-21T15:28:41Z

@martinwicke This is ready to be merged. You added the API review tag. What needs to be done?

rmlarsen · 2017-06-23T15:36:39Z

@lakshayg could you please rebase to resolve conflicts?

rmlarsen · 2017-06-23T15:37:40Z

@martinwicke I missed your earlier comment about API review last monday. I'll assign you on this PR and you can merge when it's approved.

lakshayg · 2017-06-23T15:44:40Z

@rmlarsen rebase done!

rmlarsen · 2017-06-23T15:50:54Z

@lakshayg Thanks!
@tensorflow-jenkins test this please

jhseu · 2017-06-24T00:51:57Z

How about renaming to arcsinh, arccosh, arctanh to match the numpy names?

lakshayg · 2017-06-24T00:56:03Z

@jhseu I guess then we would have to rename asin, acos and atan as well.

jhseu · 2017-06-24T01:00:02Z

Yes, but perhaps wait for API review for the final word.

rmlarsen · 2017-06-26T15:36:15Z

@jhseu We already have atan, atan2, asin etc. so it doesn't make sense unless we change the existing interface.

drpngx · 2017-06-26T21:33:42Z

@jhseu or @martinwicke : to confirm, good to go?

martinwicke · 2017-06-26T22:56:13Z

Good for API review, but watch out, this will require Eigen changes to pull in.

gunan · 2017-06-30T20:38:26Z

I see failures in windows CPU and GPU build due to additions from this change:
http://ci.tensorflow.org/job/tensorflow-master-win-cmake-py/1101/console

cwise_ops_test.py", line 411, in testComplex64Basic
cwise_ops_test.py", line 302, in testDoubleBasic
cwise_ops_test.py", line 208, in testFloatBasic

Looks like there is some flakiness in arccosh implementation on windows?

drpngx · 2017-06-30T20:54:32Z

@rmlarsen is out next week.

It was green when I merged. Looking at the code, there doesn't seem to be anything obviously special about cosh.

@lakshayg could you take a look?

drpngx · 2017-06-30T20:58:28Z

tensorflow/core/kernels/cwise_op_acosh.cc

+REGISTER_SYCL_KERNEL(float);
+REGISTER_SYCL_KERNEL(double);
+#undef REGISTER_SYCL_KERNEL
+#endif // TENSORFLOW_USE_SYC


We should fix this typo in another PR.

~~Sent in a PR for the typo SYC -> SYCL #11204~~ fixed!

lakshayg · 2017-07-01T06:28:12Z

It was suggested earlier #10598 (comment) that I change the tolerances for those but since the tests passed on the next build, I did not change the tolerances. Should I go ahead and change them? @gunan @drpngx @martinwicke

drpngx · 2017-07-01T17:09:49Z

It's not just the tolerance, check this out:

21:10:19 <class 'numpy.ndarray'> <class 'numpy.ndarray'> <class 'tensorflow.python.ops.variables.Variable'> <class 'tensorflow.python.ops.variables.Variable'>
21:10:19 <class 'tensorflow.python.framework.ops.Tensor'> <class 'tensorflow.python.framework.ops.Tensor'>
21:10:19 not close where =  (array([7, 7], dtype=int64), array([6, 7], dtype=int64))
21:10:19 not close lhs =  [ 1.  0.]
21:10:19 not close rhs =  [    0.          1570.79632679]
21:10:19 not close dif =  [  1.00000000e+00   1.57079633e+03]
21:10:19 not close tol =  [  1.00000000e-05   1.57179633e-02]

lakshayg · 2017-07-02T04:05:47Z

tensorflow/core/kernels/cwise_ops.h

+template<typename T> struct scalar_acosh_op {
+  EIGEN_EMPTY_STRUCT_CTOR(scalar_acosh_op)
+  EIGEN_DEVICE_FUNC EIGEN_STRONG_INLINE const T operator()(const T& a) const {
+    return std::acosh(a);


It's surprising that everything works fine on Linux and fails on Windows. It is extremely unlikely but could it have something to do with the standard library implementation of acosh, since that is what is used in the background for computation.

…ensorflow#10598) * Implemented asinh op * Implemented acosh op * Fixed test functions for acosh and asinh * Implemented atanh op * Fixed test cases for atanh

googlebot added the cla: yes label Jun 9, 2017

lakshayg changed the title ~~Implemented asinh and acosh operations (#7531)~~ Implements asinh and acosh operations (Partially fixes #7531) Jun 9, 2017

martinwicke changed the title ~~Implements asinh and acosh operations (Partially fixes #7531)~~ Implements asinh and acosh operations (partial fix for #7531) Jun 13, 2017

martinwicke requested a review from rmlarsen June 13, 2017 20:17

martinwicke assigned rmlarsen Jun 13, 2017

martinwicke added awaiting testing (then merge) awaiting review Pull request awaiting review and removed awaiting testing (then merge) labels Jun 13, 2017

rmlarsen approved these changes Jun 14, 2017

View reviewed changes

rmlarsen added awaiting testing (then merge) and removed awaiting review Pull request awaiting review labels Jun 14, 2017

martinwicke added the API review API Review label Jun 14, 2017

martinwicke added the stat:awaiting response Status - Awaiting response from author label Jun 14, 2017

lakshayg mentioned this pull request Jun 14, 2017

Hyperbolic functions #7531

Closed

lakshayg changed the title ~~Implements asinh and acosh operations (partial fix for #7531)~~ Implements inverse hyperbolic operations (fix for #7531) Jun 17, 2017

rmlarsen approved these changes Jun 19, 2017

View reviewed changes

rmlarsen assigned martinwicke Jun 23, 2017

lakshayg added 5 commits June 23, 2017 21:13

Implemented asinh op

f4444c0

Implemented acosh op

6f0fe11

Fixed test functions for acosh and asinh

0f6483c

Implemented atanh op

cc60a49

Fixed test cases for atanh

76de4d3

rmlarsen removed the stat:awaiting response Status - Awaiting response from author label Jun 23, 2017

martinwicke removed the API review API Review label Jun 26, 2017

drpngx merged commit cf18c6d into tensorflow:master Jun 29, 2017

drpngx reviewed Jun 30, 2017

View reviewed changes

lakshayg commented Jul 2, 2017

View reviewed changes

gunan mentioned this pull request Jul 3, 2017

Disable flaky cwise_ops_test due to broken acosh on windows. #11213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements inverse hyperbolic operations (fix for #7531) #10598

Implements inverse hyperbolic operations (fix for #7531) #10598

lakshayg commented Jun 9, 2017 •

edited by martinwicke

tensorflow-jenkins commented Jun 9, 2017

caisq commented Jun 11, 2017

lakshayg commented Jun 12, 2017 •

edited

rmlarsen commented Jun 14, 2017

lakshayg commented Jun 14, 2017

martinwicke commented Jun 14, 2017

martinwicke commented Jun 14, 2017

lakshayg commented Jun 14, 2017

martinwicke commented Jun 14, 2017

lakshayg commented Jun 14, 2017 •

edited

martinwicke commented Jun 14, 2017

martinwicke commented Jun 16, 2017

lakshayg commented Jun 17, 2017

lakshayg commented Jun 17, 2017

rmlarsen commented Jun 20, 2017

lakshayg commented Jun 21, 2017

rmlarsen commented Jun 21, 2017

rmlarsen commented Jun 23, 2017

rmlarsen commented Jun 23, 2017 •

edited

lakshayg commented Jun 23, 2017

rmlarsen commented Jun 23, 2017

jhseu commented Jun 24, 2017

lakshayg commented Jun 24, 2017

jhseu commented Jun 24, 2017

rmlarsen commented Jun 26, 2017

drpngx commented Jun 26, 2017

martinwicke commented Jun 26, 2017

gunan commented Jun 30, 2017

drpngx commented Jun 30, 2017

drpngx Jun 30, 2017

lakshayg Jul 1, 2017 •

edited

lakshayg commented Jul 1, 2017 •

edited

drpngx commented Jul 1, 2017

lakshayg Jul 2, 2017 •

edited

Implements inverse hyperbolic operations (fix for #7531) #10598

Implements inverse hyperbolic operations (fix for #7531) #10598

Conversation

lakshayg commented Jun 9, 2017 • edited by martinwicke

tensorflow-jenkins commented Jun 9, 2017

caisq commented Jun 11, 2017

lakshayg commented Jun 12, 2017 • edited

rmlarsen commented Jun 14, 2017

lakshayg commented Jun 14, 2017

martinwicke commented Jun 14, 2017

martinwicke commented Jun 14, 2017

lakshayg commented Jun 14, 2017

martinwicke commented Jun 14, 2017

lakshayg commented Jun 14, 2017 • edited

martinwicke commented Jun 14, 2017

martinwicke commented Jun 16, 2017

lakshayg commented Jun 17, 2017

lakshayg commented Jun 17, 2017

rmlarsen commented Jun 20, 2017

lakshayg commented Jun 21, 2017

rmlarsen commented Jun 21, 2017

rmlarsen commented Jun 23, 2017

rmlarsen commented Jun 23, 2017 • edited

lakshayg commented Jun 23, 2017

rmlarsen commented Jun 23, 2017

jhseu commented Jun 24, 2017

lakshayg commented Jun 24, 2017

jhseu commented Jun 24, 2017

rmlarsen commented Jun 26, 2017

drpngx commented Jun 26, 2017

martinwicke commented Jun 26, 2017

gunan commented Jun 30, 2017

drpngx commented Jun 30, 2017

drpngx Jun 30, 2017

Choose a reason for hiding this comment

lakshayg Jul 1, 2017 • edited

Choose a reason for hiding this comment

lakshayg commented Jul 1, 2017 • edited

drpngx commented Jul 1, 2017

lakshayg Jul 2, 2017 • edited

Choose a reason for hiding this comment

lakshayg commented Jun 9, 2017 •

edited by martinwicke

lakshayg commented Jun 12, 2017 •

edited

lakshayg commented Jun 14, 2017 •

edited

rmlarsen commented Jun 23, 2017 •

edited

lakshayg Jul 1, 2017 •

edited

lakshayg commented Jul 1, 2017 •

edited

lakshayg Jul 2, 2017 •

edited