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

MATLAB naming in API #1300

Closed
jaeandersson opened this Issue Jan 8, 2015 · 19 comments

Comments

Projects
None yet
3 participants
@jaeandersson
Member

jaeandersson commented Jan 8, 2015

There are a number of functions in the API that could be made more MATLAB-like:

  • Boolean queries in lower case instead of CamelCase: issparse instead of isSparse, isdiag instead of isDiag etc.
  • full, isfull instead of dense, isDense
  • blkdiag instead of diagcat. (I replaced all instances of blkdiag but that might have been a mistake)
  • size returning shape, not number of nonzeros
  • nnz returning number of nonzeros
  • nonzeros instead of data (I'm adding nonzeros, but it is not a replacement, since the return type is different).
  • iscolumn instead of isVector (in MATLAB, isvector is true for row and column vectors.)

There are probably many more. Not sure if all changes makes sense, though. And changing the API might annoy users... On the other hand, it makes the syntax more familiar for MATLAB users.

@jgillis

This comment has been minimized.

Member

jgillis commented Jan 8, 2015

Is it correct that the examples you posted would correspond to global functions in matlab?
How about just adding those to the matlab interface and make them use the existing camelcase member functions?

@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 8, 2015

It is possible to use a different syntax in MATLAB, but the question is if it would be desirable. Having the same MATLAB-like syntax makes the learning curve less steep and avoids the confusions you get from having different syntaxes in different languages.

@jgillis

This comment has been minimized.

Member

jgillis commented Jan 8, 2015

Chaning size would definitely be annoying.
The others can all be done with gentle deprecations

@ghorn

This comment has been minimized.

Member

ghorn commented Jan 8, 2015

is the naming now based on numpy?

@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 8, 2015

The naming now is "MATLAB inspired". But many things are not really planned. Matrix.size() is named this way since many many iterations ago, Matrix was inheriting from std::vector...

@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 8, 2015

About size(), I'm also not sure about it. Anyway, the first step is to implement the sparsity iterators (#57)...

@ghorn

This comment has been minimized.

Member

ghorn commented Jan 8, 2015

I don't mind the C++ API to have matlab-like names, and no camel-case. I'm not sure how python users would feel.

My biggest worry would be having a function name which exactly matches matlab, but works subtly different on a matlab matrix than on a casadi matrix. (Or causes overloading issues)

@jaeandersson jaeandersson referenced this issue Jan 10, 2015

Closed

More tidying up of API #1302

10 of 13 tasks complete
@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 10, 2015

I would also like to deprecate the syntax Y = SX.repmat(X, ...). It is enough to have Y = repmat(X, ...) with type of Y inferred from X. X can always be cast into the correct type to avoid ambiguity, e.g. Y = repmat(SX(4), 5, 6). We could also define a repmat(double, ...) that returns a DMatrix (not needed in MATLAB).

@jaeandersson jaeandersson added this to the Version 2.3.0 milestone Jan 10, 2015

@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 23, 2015

Is it correct that the examples you posted would correspond to global functions in matlab?
How about just adding those to the matlab interface and make them use the existing camelcase member functions?

Thought I should clarify this: No, all the functions above corresponds to member functions in MATLAB. If you have a member function "issparse", that you would call using the syntax "x.issparse()" in C++, the MATLAB way of calling it would be 'issparse(x)". You don't want to have a global function called "issparse", because then this will be called instead of the built-in one when working with MATLAB built-in datatypes.

jaeandersson added a commit that referenced this issue Jan 23, 2015

#1300 Deprecated MX::repmat, Matrix::repmat.
Syntax is now (only) repmat(MX, ...), repmat(Matrix, ...)
@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 23, 2015

I'd like to go with the following convention:

  • densify(A) returns a dense(r) CasADi matrix of the same type as A
  • full(A) returns a dense MATLAB matrix when in MATLAB, it will be the standard way of converting CasADi type to dense MATLAB matrix. In Python, we could use the same syntax for returning a 2-dimensional numpy array.
  • sparsify(A) returns a sparse(r) CasADi matrix of the same type as A
  • sparse(A) returns a sparse MATLAB matrix when in MATLAB, it will be the standard way of converting CasADi type to sparse MATLAB matrix. In Python, we could use the same syntax for returning a scipy.csc_matrix.
  • Sparsity.dense(n,m), Sparsity.sparse(n, m) will continue to work as now for constructing dense/sparse sparsity patterns.
@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 23, 2015

@jgillis - any objections?

@jgillis

This comment has been minimized.

Member

jgillis commented Jan 24, 2015

what would full and sparse do for the sxmatrix and mx type? break doen into 1-by-1 s?

@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 24, 2015

Convert to numeric sparse/dense matrix, throw error if not possible.

@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jan 24, 2015

full(A) in matlab gives a dense numeric matrix for A dense or sparse matlab matrices. makes sense to have casadi matrices behave the same way. same with sparse.

@jgillis jgillis closed this Jan 24, 2015

@jgillis jgillis reopened this Jan 24, 2015

@jgillis

This comment has been minimized.

Member

jgillis commented Jan 24, 2015

you seem very determined so go ahead. make sure to put some \see in the docstrings..

jaeandersson added a commit that referenced this issue Jan 24, 2015

jaeandersson added a commit that referenced this issue Jan 24, 2015

#1300 Removed deprecated sparse(Matrix).
Syntax is now sparsify(Matrix)

jaeandersson added a commit that referenced this issue Jan 24, 2015

#1300 Renamed Sparsity::size()-->nnz(), GenericMatrix::size()-->nnz()
Old syntax should give deprecation warning in next release.
Replaced occurences in C++

jaeandersson added a commit that referenced this issue Jan 24, 2015

jaeandersson added a commit that referenced this issue Jan 24, 2015

jaeandersson added a commit that referenced this issue Jan 24, 2015

jaeandersson added a commit that referenced this issue Jan 24, 2015

jaeandersson added a commit that referenced this issue Jan 24, 2015

jaeandersson added a commit that referenced this issue Jan 26, 2015

#1300 Unexposed Sparsity.sparse(n,m), GenericMatrix.sparse(n,m) in MA…
…TLAB.

Enabled syntax sparse(A) for converting to MATLAB sparse matrix

jaeandersson added a commit that referenced this issue Jan 26, 2015

jaeandersson added a commit that referenced this issue Jun 29, 2015

issue #1300: removed deprecated Matrix.size()
The syntax is now Matrix.nnz()

jaeandersson added a commit that referenced this issue Jun 29, 2015

jaeandersson added a commit that referenced this issue Jun 29, 2015

issue #1300: Replaced isVector with isvector
isvector corresponds to isVector(true), i.e. isrow or iscolumn
@jaeandersson

This comment has been minimized.

Member

jaeandersson commented Jun 29, 2015

Closing this issue here. I left diagcat as I can see some issues with blkdiag: it would make the syntax diagsplit illogical and breaks the pattern with vertcat, horzcat.
I also removed size() but postponed the renaming of shape() to size() for later to avoid weird errors.

@jaeandersson jaeandersson self-assigned this Jun 29, 2015

jaeandersson added a commit that referenced this issue Jun 29, 2015

jaeandersson added a commit that referenced this issue Jun 29, 2015

jaeandersson added a commit that referenced this issue Jun 29, 2015

jaeandersson added a commit that referenced this issue Jun 29, 2015

jgillis added a commit that referenced this issue Jun 29, 2015

jgillis added a commit that referenced this issue Jul 18, 2015

@jaeandersson jaeandersson referenced this issue Sep 25, 2015

Closed

Syntax cleanup for version 3.0 #1586

12 of 14 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment