-
Notifications
You must be signed in to change notification settings - Fork 955
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 11 files at r1, 10 of 10 files at r3.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @caisq)
src/environment_util.ts, line 165 at r3 (raw file):
gl.readPixels(0, 0, 1, 1, gl.RGBA, gl.FLOAT, new Float32Array(4)); const readPixelsNoError = gl.getError() === gl.NO_ERROR;
If I remember correctly this was purposeful, can you just make sure iOS works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @caisq)
src/environment_util.ts, line 165 at r3 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
If I remember correctly this was purposeful, can you just make sure iOS works?
iOS works because the the above added check if (!hasExtension(gl, 'WEBGL_color_buffer_float')) {
which fails on iOS.
The motivation for this change is debugging numerical issues with Adam (and other optimizers) on phones.
backend.floatPrecision()
which does feature testing usingabs(epsilon) > 0
(epsilon is a small number) to determine if the webgl is using 16bit or 32bit floats. This test works on Android and iPhones pointing to 16bit arithmetic.EPSILON
which points to the smallest positive number used to make ops numerically stable (e.g.div(x + eps)
,log(x + eps)
, etc). The value depends onbackend.floatPrecision()
.TEST_EPSILON
now depends onbackend.floatPrecision()
as well. This makes unit tests pass on Android devices as well.ENV.get('EPSILON')
expectArraysClose()
which needs a backend in order to determine TEST_EPSILONhasExtension(gl, 'WEBGL_color_buffer_float')
as a test instead. This removes the WEBGL_ERROR message in Safari.@caisq, a followup would be to have K.epsilon in tfjs-layers point to ENV.get('EPSILON') and do further testing on the iris example with the Adam Optimizer (e.g. there is a call to log(x) in categorical cross entropy which can be stabilized by calling log(x+eps) instead.)
BUG
This change is