Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Fix failing test in travis #1381

Merged
merged 3 commits into from
Nov 7, 2018
Merged

Fix failing test in travis #1381

merged 3 commits into from
Nov 7, 2018

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Nov 7, 2018

#1379 added a unit test that failed in travis due to numerical precision: https://travis-ci.org/tensorflow/tfjs-core/jobs/451654174#L801

This PR:


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @annxingyuan)

@nsthorat
Copy link
Contributor

nsthorat commented Nov 7, 2018

Looks like those bash comments are screwing things up, my guess is because of the \ happening before the comment.

Copy link
Contributor Author

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Yeah, removed them.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan)

@dsmilkov dsmilkov merged commit 10ebb98 into master Nov 7, 2018
@dsmilkov dsmilkov deleted the fix-break branch November 7, 2018 14:38
@DirkToewe
Copy link
Contributor

Out of curiosity: Why is the Safari JSVM behaving differently? The results seem to be way of. Even if summed up naively:

Array.from({ length: 10*1000 }, () => Math.fround(0.1**2) )
     .reduce( (x,y) => Math.fround(x+y), /*initVal=*/0.0 )

The result on my machine is 100.0029525756836. But if I am not mistaken the CPU matrix multiplication in TFJS is blocked up. Inside the blocks, a float64 value is used to sum the values up. So the result should be something along the lines of:

const blockSize = 50;
Array.from(
  { length: 10*1000/blockSize },
  () => Math.fround(
    Array.from({ length: blockSize }, () => Math.fround(0.1**2) )
         .reduce( (x,y) => x+y, /*initVal=*/0.0 )
  )
).reduce( (x,y) => Math.fround(x+y), /*initVal=*/0.0 )

That result even perfectly rounds to 100 on my machine.

In the Travis log the result seems to be 99.29019927978516 which would be way off.

@dsmilkov
Copy link
Contributor Author

dsmilkov commented Nov 7, 2018

I noticed the same - Didn't give much thought since that test was not failing on my local laptop (Macbook Pro). The only diff between my laptop and Travis was the Safari version: mine is 12.0.1, Travis used 11.1.2

@DirkToewe
Copy link
Contributor

I too had some issued with the Safari tests on Travis. My guess was that it was a WebGL thing, but maybe there is an issue with Float32Array in that Webkit/Safari version?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants