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

ti.ndrange does not check for number of parameters #6351

Closed
houkensjtu opened this issue Oct 17, 2022 · 8 comments · Fixed by #6422
Closed

ti.ndrange does not check for number of parameters #6351

houkensjtu opened this issue Oct 17, 2022 · 8 comments · Fixed by #6422
Assignees
Labels
potential bug Something that looks like a bug but not yet confirmed

Comments

@houkensjtu
Copy link
Contributor

houkensjtu commented Oct 17, 2022

Describe the bug
ti.ndrange does not report an error or warning even when the number of parameters does not match.

To Reproduce

import taichi as ti

ti.init(arch=ti.cpu)

@ti.kernel
def iter():
    for i in ti.ndrange(1, nx):  # Taichi scope
        print(i)

iter()  # 0,1,2,3,4,5,6,7

for i in ti.ndrange(1, nx):  # Python scope
    print(i)  # (0,0) (0,1) (0,2) (0,3) (0,4) (0,5) (0,6) (0,7)

As shown in the code example above, a user who is unfamiliar with ti.ndrange's syntax might expect the printing result of iter() to be 1, 2, 3, 4, 5, 6, 7; however, since ti.ndrange requires a parentheses for each index and the users forgot to include one, the results might be unexpected. It's probably better to give a warning or error message when the number of index does not match the parameters in ti.ndrange.

It's also confusing that the same syntax yields different results for Taichi scope and Python scope. If this kind of mismatch is allowed (with a warning but not an error), then the current behavior in Python scope seems to be more reasonable than the results in Taichi scope.

@lin-hitonami
Copy link
Contributor

In fact the use of the ndrange-for in Python scope is a use of grouped ndrange-for... I've come up with an idea that we treat the use of ndrange-for with only one loop variable but has many dimensions automatically as a grouped ndrange-for (maybe we can do the same thing on the struct for too). In this way the syntax of Python and Taichi can be united, and the ti.grouped decorator can be deprecated. WDYT @yuanming-hu @ailzhang @strongoier

@yuanming-hu
Copy link
Member

For ndrange deprecating ti.grouped may be an option.

For struct-fors, if we deprecate ti.grouped, what would be the type for i in for i in x where x is a 1-D field? Is that a scalar or a 1D vector? :-)

If we only deprecate it for ndrange then there may be some inconsistency between two for loops.

@lin-hitonami
Copy link
Contributor

lin-hitonami commented Oct 18, 2022

For struct-fors, if we deprecate ti.grouped, what would be the type for i in for i in x where x is a 1-D field? Is that a scalar or a 1D vector? :-)

What about a subclass of ti.Expr which can have subscript? We can let i[0] return i?

@yuanming-hu
Copy link
Member

yuanming-hu commented Oct 18, 2022

TBH, that would be a little tricky to understand by users, given it's neither a scalar nor a vector...

@lin-hitonami
Copy link
Contributor

lin-hitonami commented Oct 18, 2022

TBH, that would be a little tricky to understand by users, given it's neither a scalar nor a vector...

It is actually a scalar and a vector (with one element).

@lin-hitonami
Copy link
Contributor

Given this only happens when there is only one loop variable and the field has only one dimension, I think it is not that hard to understand......

@yuanming-hu
Copy link
Member

While it may be understandable by some of the users, it does break the uniformity. If we use ti.grouped then it would be crystal clear that you get a vector when you use ti.grouped, and scalars when you don't.

lin-hitonami added a commit that referenced this issue Oct 18, 2022
…not match the dimension of the ndrange (#6360)

Issue: #6351 

### Brief Summary

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@strongoier
Copy link
Contributor

I agree that we should not mix scalar and vector together.

jim19930609 pushed a commit to jim19930609/taichi that referenced this issue Oct 19, 2022
…not match the dimension of the ndrange (taichi-dev#6360)

Issue: taichi-dev#6351 

### Brief Summary

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@feisuzhu feisuzhu moved this from Untriaged to Todo in Taichi Lang Oct 21, 2022
lin-hitonami added a commit that referenced this issue Oct 25, 2022
…mension of the ndrange (#6422)

Issue: fixes #6351 

### Brief Summary

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Repository owner moved this from Todo to Done in Taichi Lang Oct 25, 2022
jim19930609 pushed a commit to jim19930609/taichi that referenced this issue Oct 25, 2022
…mension of the ndrange (taichi-dev#6422)

Issue: fixes taichi-dev#6351

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential bug Something that looks like a bug but not yet confirmed
Projects
Status: Done
5 participants