Skip to content

Conversation

@xhcao
Copy link
Contributor

@xhcao xhcao commented Dec 9, 2022

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@xhcao xhcao requested review from gyagp and qjia7 December 9, 2022 08:35
var p3 = fract(vec3<f32>(p.xyx) * HASHSCALE1);
p3 = p3 + dot(p3, p3.yzx + 19.19);
return fract((p3.x + p3.y) * p3.z);
}
Copy link

Choose a reason for hiding this comment

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

We can put this in shader_util.ts, so that other code may share.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function could be only used for 2D coordinate axes. If we want to move it to shader_util.ts, we should design a more common function for 1~4D coordinate axes on webgpu backend. Is it OK to do it in the future if necessary?

Copy link

Choose a reason for hiding this comment

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

Sure, we can make it a util function when necessary in the future.

f32(coords[0]) / f32(uniforms.outShape[0]));
let r = random(uniforms.seed, resUV);
var cdf = 0.0;
for (var i = 0; i < uniforms.numOutcomes; i = i + 1) {
Copy link

Choose a reason for hiding this comment

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

In WebGL impl, it's (uniforms.numOutcomes -1) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for finding the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easy to add a case to catch such kind of error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

r is random value, I think it is difficult to design the case.

@xhcao
Copy link
Contributor Author

xhcao commented Jan 3, 2023

@gyagp PTAL

@xhcao xhcao merged commit 70183e4 into tensorflow:master Jan 4, 2023
Linchenn pushed a commit to Linchenn/tfjs that referenced this pull request Jan 9, 2023
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