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鈥檛 exceed 5-10, or you鈥檙e 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鈥檙e brilliant, but maybe you鈥檇 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 https://github.com/taichi-dev/taichi/pull/1046#issue-422369389.
Note that we should always try our best to make a well-polished matrix/vector API before Taichi v1.0.0 comes.
I presume that this needs to be done before June 13th for GAMES201 ?
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?
Most helpful comment
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 doMatrix.varafter GAMES201 in v0.7.0?