Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lang] ti.sqr(x) is now deprecated: please use x ** 2 instead #937

Merged
merged 2 commits into from
May 9, 2020

Conversation

yuanming-hu
Copy link
Member

@yuanming-hu yuanming-hu commented May 9, 2020

Related issue = #833

People tend to think sqr means square root instead of square (https://forum.taichi.graphics/t/what-the-result-of-ti-sqr-x-x-is-a-vector-if-any-element-of-x-is-negative/125/3).

Fortunately, we have ** 2 now thanks to @archibate...

[Click here for the format server]

@yuanming-hu yuanming-hu requested a review from archibate May 9, 2020 16:24
@yuanming-hu yuanming-hu changed the title [Lang] ti.sqr(x) is now deprecated. Please use x ** 2 instead. [Lang] ti.sqr(x) is now deprecated: please use x ** 2 instead May 9, 2020
@archibate
Copy link
Collaborator

archibate commented May 9, 2020

sgtm, thx, also consider:
[x] remove ti.sqr from doc.
[ ] import deprecated, deprecate ti.sqr.
[ ] remove ti.sqr from lang/ops.py.

@yuanming-hu
Copy link
Member Author

Thanks. It's actually already deprecated:
https://github.com/taichi-dev/taichi/pull/937/files#diff-d13b207c22d2668af6db2c8d635648e9R81-R83

Given there are quite a few other repos using ti.sqr, I think we should keep it there for a while and then remove it.

Copy link
Collaborator

@archibate archibate left a comment

Choose a reason for hiding this comment

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

Thank for the hard work! I believe you've grep -wr 'ti.sqr' examples'ed, if so, LGTM now.

@archibate
Copy link
Collaborator

Feel free to merge this now.
Btw, what does ti.inv and mean? Can we remove that too?
Also found that UnaryOpType::rcp and UnaryOpType::rsqrt are not used at all, can we remove them? (I guess rcp is the same meaning as inv: 1/x..)

@yuanming-hu
Copy link
Member Author

Thanks. inv is for inverse. Yes, maybe we should remove it since it's confusing. We should consider exposing rcp and rsqrt. Let's move the discussions to #833

@yuanming-hu yuanming-hu merged commit 62868a0 into taichi-dev:master May 9, 2020
@k-ye
Copy link
Member

k-ye commented May 10, 2020

I have no idea why, but this PR made sdf_renderer.py on Metal ~8x times slower... In literally the previous commit 8277acf, I still got 24 samples/s, now it's around 3.5 samples/s... @yuanming-hu @archibate @xumingkuan could you help test on the CUDA/OpenGL backends as well?

@archibate
Copy link
Collaborator

archibate commented May 10, 2020

8x slower...

me too. I tweaked norm_sqr and found:
CUDA: 4.5sps vs 1.2sps, ~4x slower.
OpenGL: 6.8sps vs error: 0(68) : error C1503: undefined variable "nan".

Seems it's boardcast_if_scalar harms the perf? Could you also checkout the constant fold #839 and see if that helps?

@archibate
Copy link
Collaborator

I realized why! Because boardcast_if_scalar cast the 2 into 2.0, which disables unrolling (i.e. use pow_f32 instead of pow_i32)
I think self**2 -> self * self will solve this.
Also refactor boardcast_if_scalar can solve this.

@k-ye
Copy link
Member

k-ye commented May 10, 2020

I realized why! Because boardcast_if_scalar cast the 2 into 2.0, which disables unrolling (i.e. use pow_f32 instead of pow_i32)

Ah i see, thx! Yep I changed to self * self and it's fast again!

Could you also checkout the constant fold #839 and see if that helps?

Unfortunately no, but i guess it's not supposed to help in this case...

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