add Rint operation #4113

Mistobaan · 2016-08-31T01:56:13Z

Based on previous work: https://github.com/tensorflow/tensorflow/pull/1288/files
and from a comment on #3730. Fixes #3730.

tensorflow-jenkins · 2016-08-31T01:56:13Z

Can one of the admins verify this patch?

martinwicke · 2016-08-31T16:29:49Z

There are conflicts, have you rebased already?

vrv · 2016-08-31T16:30:49Z

tensorflow/core/ops/compat/ops_history.v0.pbtxt

@@ -19373,6 +19373,27 @@ op {
  }
 }
 op {
+  name: "Rint"


you can drop this file

Mistobaan · 2016-09-01T21:10:45Z

@martinwicke @girving I addressed all the changes proposed.

girving · 2016-09-02T03:25:33Z

tensorflow/core/kernels/cwise_op_gpu_rint.cu.cc

+
+#include "tensorflow/core/kernels/cwise_ops_gpu_common.cu.h"
+
+


Only one blank line.

rmlarsen · 2016-09-03T23:15:30Z

One thing to keep in mind is that std::round sets the rounding mode on every call and is significantly slower than std::rint, so it's nice to have this op.

martinwicke · 2016-11-03T17:54:07Z

I think you have to add the added .cc files to the list of files compiled by makefile.

@petewarden to confirm where exactly.

petewarden · 2016-11-03T17:56:02Z

Looks like 'tensorflow/core/platform/setround.cc' isn't being linked in by the makefile. This is surprising, because I would expect the wildcard here to include it in the sources:
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/makefile/Makefile#L423

I also see other files in tensorflow/core/platform being compiled and linked in, looking at the logs:
https://ci.tensorflow.org/job/tensorflow-pull-requests-makefile/6255/consoleFull

Can you try adding $(warning $TF_CC_SRCS) around line 467 in the Makefile to show what we've calculated? If that's tricky, I can dig into it myself later today.

vrv · 2016-11-04T20:54:11Z

ping for @petewarden and @Mistobaan

vrv · 2016-11-09T00:19:48Z

Friendly ping -- it would be lovely to get this in.

Mistobaan · 2016-11-13T04:10:12Z

@vrv I was able to reproduce and fix the error ( setround.cc had to be manually added to a dependency text file).

vrv · 2016-11-13T05:18:44Z

@tensorflow-jenkins test this please

(WoohoooooO!)

Mistobaan · 2016-11-13T08:15:46Z

now the windows cmake build is failing ... I can't help with that. I don't have a win machine.

drpngx · 2016-11-14T05:50:19Z

Strange, it cannot import tensorflow. Let's see if it was a transient issue.

Jenkins, test this please.

drpngx · 2016-11-14T05:50:59Z

tensorflow/core/ops/math_ops.cc

+For example:
+
+```
+rint(-1.5) ==> 0.0


Is this right?

drpngx · 2016-11-14T05:51:30Z

tensorflow/python/kernel_tests/cwise_ops_test.py

@@ -1790,9 +1798,15 @@ def _compare(self, x, use_gpu):
    self.assertShapeEqual(np_ceil, oceil)

  def _testDtype(self, dtype):
-    data = (np.arange(-3, 3) / 4.).reshape([1, 3, 2]).astype(dtype)


How come we have to modify current test behavior?

see previous comments. the test is unchanged.

drpngx · 2016-11-14T05:52:38Z

tensorflow/python/ops/math_ops.py

@@ -59,6 +59,7 @@
 @@log1p
 @@ceil
 @@floor
+@@rint


We're trying to have numpy behavior @aselle

Could you add this example to the unit test?
https://docs.scipy.org/doc/numpy/reference/generated/numpy.rint.html

Mistobaan · 2016-11-16T02:15:48Z

@drpngx rebased and addressed the proposed changes.

drpngx · 2016-11-16T02:29:54Z

Jenkins, test this please.

drpngx · 2016-11-16T02:32:19Z

Something broken with the makefile, will have to try again later.

rmlarsen · 2016-11-16T16:34:57Z

@gunan it looks like the makefile issue is unrelated. Can we merge this?

gunan · 2016-11-16T16:40:43Z

Yes, the failure looks like a flake. We can go ahead and merge.

carlorosati · 2017-02-06T13:13:39Z

Is it really std::fesetround or is it just fesetround?

I received the following ERROR when building.

ERROR: /local/tmp/tensorflow/tensorflow/core/BUILD:1036:1: C++ compilation of rule '//tensorflow/core:lib_internal' failed: gcc failed: error executing command /local2/tmp/brew/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -B/local2/tmp/brew/bin -B/usr/bin -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -g0 -O2 ... (remaining 104 argument(s) skipped): com.google.devtools.build.lib.shell.BadExitStatusException: Process exited with status 1.
tensorflow/core/platform/setround.cc: In constructor 'tensorflow::port::ScopedSetRound::ScopedSetRound()':
tensorflow/core/platform/setround.cc:27:4: error: 'fesetround' is not a member of 'std'
    std::fesetround(FE_TONEAREST);
    ^
tensorflow/core/platform/setround.cc:27:4: note: suggested alternative:
In file included from tensorflow/core/platform/setround.cc:19:0:
/local2/tmp/brew/include/fenv.h:88:12: note:   'fesetround'
 extern int fesetround (int __rounding_direction) __THROW;
            ^
Target //tensorflow/tools/pip_package:build_pip_package failed to build

EDIT:
On further inspection, the problem seems to be that gcc is being used to compile a C++ file.. I have opened issue #7289

googlebot added the cla: yes label Aug 31, 2016

Mistobaan mentioned this pull request Aug 31, 2016

tf.round != np.round #3730

Closed

vrv reviewed Aug 31, 2016
View reviewed changes

tensorflow/core/ops/compat/ops_history.v0.pbtxt

@@ -19373,6 +19373,27 @@ op {

}

}

op {

name: "Rint"

Copy link

vrv Aug 31, 2016

Choose a reason for hiding this comment

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

you can drop this file

Mistobaan force-pushed the features/rint-op branch 2 times, most recently from 3cacde5 to fc5806b Compare September 1, 2016 21:09

maciekcc assigned girving Sep 2, 2016

girving reviewed Sep 2, 2016
View reviewed changes

tensorflow/core/kernels/cwise_op_gpu_rint.cu.cc

#include "tensorflow/core/kernels/cwise_ops_gpu_common.cu.h"

Copy link

Contributor

girving Sep 2, 2016

Choose a reason for hiding this comment

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

Only one blank line.

teamdandelion added the stat:awaiting response Status - Awaiting response from author label Sep 6, 2016

vrv added stat:awaiting response Status - Awaiting response from author and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower labels Nov 3, 2016

Mistobaan force-pushed the features/rint-op branch from 0f8f661 to 651fae9 Compare November 13, 2016 04:09

drpngx suggested changes Nov 14, 2016

View reviewed changes

rmlarsen assigned drpngx and unassigned rmlarsen Nov 14, 2016

Mistobaan added 4 commits November 15, 2016 16:57

add rint op

874caa9

wip on custom rounding op

54c7072

fix Makefile build for the rint op

c3d74f1

rebase, add test, fix examples

9696341

Mistobaan force-pushed the features/rint-op branch from 651fae9 to 9696341 Compare November 16, 2016 01:53

drpngx approved these changes Nov 16, 2016

View reviewed changes

drpngx added awaiting testing (then merge) and removed stat:awaiting response Status - Awaiting response from author labels Nov 16, 2016

rmlarsen merged commit 8f32013 into tensorflow:master Nov 16, 2016

Mistobaan deleted the features/rint-op branch January 22, 2017 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Rint operation #4113

add Rint operation #4113

Mistobaan commented Aug 31, 2016 •

edited by martinwicke

Loading

tensorflow-jenkins commented Aug 31, 2016

martinwicke commented Aug 31, 2016

vrv Aug 31, 2016

Mistobaan commented Sep 1, 2016

girving Sep 2, 2016

rmlarsen commented Sep 3, 2016

martinwicke commented Nov 3, 2016

petewarden commented Nov 3, 2016

vrv commented Nov 4, 2016

vrv commented Nov 9, 2016

Mistobaan commented Nov 13, 2016

vrv commented Nov 13, 2016

Mistobaan commented Nov 13, 2016

drpngx commented Nov 14, 2016

drpngx Nov 14, 2016

drpngx Nov 14, 2016

Mistobaan Nov 16, 2016

drpngx Nov 14, 2016

Mistobaan commented Nov 16, 2016

drpngx commented Nov 16, 2016

drpngx commented Nov 16, 2016

rmlarsen commented Nov 16, 2016

gunan commented Nov 16, 2016

carlorosati commented Feb 6, 2017 •

edited

Loading


		#include "tensorflow/core/kernels/cwise_ops_gpu_common.cu.h"

@@ @@ -59,6 +59,7 @@ @@
               @@log1p
               @@ceil
               @@floor
+              @@rint

add Rint operation #4113

add Rint operation #4113

Conversation

Mistobaan commented Aug 31, 2016 • edited by martinwicke Loading

tensorflow-jenkins commented Aug 31, 2016

martinwicke commented Aug 31, 2016

vrv Aug 31, 2016

Choose a reason for hiding this comment

Mistobaan commented Sep 1, 2016

girving Sep 2, 2016

Choose a reason for hiding this comment

rmlarsen commented Sep 3, 2016

martinwicke commented Nov 3, 2016

petewarden commented Nov 3, 2016

vrv commented Nov 4, 2016

vrv commented Nov 9, 2016

Mistobaan commented Nov 13, 2016

vrv commented Nov 13, 2016

Mistobaan commented Nov 13, 2016

drpngx commented Nov 14, 2016

drpngx Nov 14, 2016

Choose a reason for hiding this comment

drpngx Nov 14, 2016

Choose a reason for hiding this comment

Mistobaan Nov 16, 2016

Choose a reason for hiding this comment

drpngx Nov 14, 2016

Choose a reason for hiding this comment

Mistobaan commented Nov 16, 2016

drpngx commented Nov 16, 2016

drpngx commented Nov 16, 2016

rmlarsen commented Nov 16, 2016

gunan commented Nov 16, 2016

carlorosati commented Feb 6, 2017 • edited Loading

Mistobaan commented Aug 31, 2016 •

edited by martinwicke

Loading

carlorosati commented Feb 6, 2017 •

edited

Loading