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] make Matrix.__init__ less response by introducing multiple initializers #1095

Closed
archibate opened this issue Jun 1, 2020 · 2 comments · Fixed by #1107
Closed

[Lang] make Matrix.__init__ less response by introducing multiple initializers #1095

archibate opened this issue Jun 1, 2020 · 2 comments · Fixed by #1107
Labels
discussion Welcome discussion! feature request Suggest an idea on this project GAMES201 GAMES 201 students' wishlist good first issue A great chance for starters python Python engineering related
Milestone

Comments

@archibate
Copy link
Collaborator

Concisely describe the proposed feature
I strongly argue about the ultra-massive-centralized-multiplexer in ti.Matrix.__init__:

def __init__(self, n=1, m=1, dt=None, shape=None,
             empty=False, layout=None, needs_grad=False,
             keep_raw=False, rows=None, cols=None):
   ... # 100+ lines of if-else-if's, within just **one function**

Functions should be short and sweet, and do just one thing. They should fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24, as we all know), and do one thing and do that well.

... said the Linux Kernel Coding Style, and I believe the 80x24 rule also works in python too.

Another measure of the function is the number of local variables. They shouldn’t exceed 5-10, or you’re doing something wrong. Re-think the function, and split it into smaller pieces. A human brain can generally easily keep track of about 7 different things, anything more and it gets confused. You know you’re brilliant, but maybe you’d like to understand what you did 2 weeks from now.

Not to say lobal variables, just the arguments alone, there are 11.

Describe the solution you'd like (if any)

Also note that the API initializing a ti.Matrix is highly centralized, GAMES201 students can easily get confused by error messages when they're not using the correct argument specification:

ti.Matrix(n, m, dt, shape)  # matrix tensor
ti.Matrix(n, dt, shape)  # vector tensor
ti.Matrix([[a, b], [c, d]])  # taichi-scope matrix
ti.Matrix([a, b])  # taichi-scope vector
ti.Matrix(cols=[u, v])  # taichi-scope matrix, with col vectors
ti.Matrix(rows=[u, v])  # taichi-scope matrix, with row vectors
ti.Matrix(n, m, empty=True)  # empty matrix

Don't you think a better design could be:

ti.Matrix.var(n, m, dt, shape)  # matrix tensor
ti.Vector.var(n, dt, shape)  # vector tensor
ti.Matrix([[a, b], [c, d]])  # taichi-scope matrix
ti.Vector([a, b])  # taichi-scope vector
ti.Matrix.cols([u, v])  # taichi-scope matrix, with col vectors
ti.Matrix.rows([u, v])  # taichi-scope matrix, with row vectors
ti.Matrix.empty(n, m)  # empty matrix

to give ti.Matrix.__init__ less duty just like currently ti.Matrix.ones and ti.Matrix.zeros does?

Or at least we can make ti.Matrix.__init__ serve as a router just redirecting to the actual functions like ti.Matrix.init_as_local and ti.Matrix.init_as_var, instead of putting those implementations inside of ti.Matrix.__init__ if-else-if's?

Additional comments

As I mentioned in #1046 (comment).
Note that we should always try our best to make a well-polished matrix/vector API before Taichi v1.0.0 comes.

@archibate archibate added feature request Suggest an idea on this project python Python engineering related discussion Welcome discussion! good first issue A great chance for starters GAMES201 GAMES 201 students' wishlist labels Jun 1, 2020
@liaopeiyuan
Copy link
Contributor

liaopeiyuan commented Jun 1, 2020

I presume that this needs to be done before June 13th for GAMES201 ?

@archibate
Copy link
Collaborator Author

archibate commented Jun 1, 2020

Yes, I think so, either done this refactor before 6.13 or after GAMES201 ends, or students will be confused with deprecated APIs...

Maybe we want to do ti.Matrix.cols/rows/.. refactor first, and do Matrix.var after GAMES201 in v0.7.0?

@archibate archibate self-assigned this Jun 1, 2020
@yuanming-hu yuanming-hu mentioned this issue Jun 1, 2020
18 tasks
@archibate archibate removed their assignment Jun 2, 2020
@archibate archibate added this to the v0.6.8 milestone Jun 4, 2020
@archibate archibate linked a pull request Jun 5, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Welcome discussion! feature request Suggest an idea on this project GAMES201 GAMES 201 students' wishlist good first issue A great chance for starters python Python engineering related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants