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

RFC Rename diagmm as scale #2916

Closed
dmbates opened this issue Apr 22, 2013 · 6 comments
Closed

RFC Rename diagmm as scale #2916

dmbates opened this issue Apr 22, 2013 · 6 comments
Assignees
Labels
needs decision A decision on this change is needed

Comments

@dmbates
Copy link
Member

dmbates commented Apr 22, 2013

This suggestion came up some time ago but I can't put my hands on exactly where the discussion originated. Methods for scale! for an Array and a scalar are defined in linalg/generic.jl and linalg/dense.jl. The effect of the diagmm method for a matrix and a vector can be regarded as scaling the rows or columns of the matrix (rows if the first argument is the vector, columns if the first arg is the matrix). The suggestion is to rename diagmm! as scale! and diagmm as scale.

Part of my motivation for this suggestion is from the CHOLMOD module. The C function cholmod_scale has the capability of

  • scaling a sparse matrix by a scalar constant
  • scaling the rows of a sparse matrix
  • scaling the columns of a sparse matrix
  • scaling the rows and columns of a symmetric sparse matrix (the logical equivalent of D_A_D where A is the symmetric matrix and D is the diagonal matrix corresponding to the scaling vector).

and it makes sense to me to use multiple dispatch in Julia for a similar purpose.

@ViralBShah
Copy link
Member

This is a great idea - I never really liked the name diagmm. Should we call this scalem, along the lines of expm and such? scale can still refer to element-wise scaling in matrices, and scalem to matrix scaling.

@StefanKarpinski
Copy link
Member

There's no need for a new name since you know what to do from the type of the argument – scalar or vector.

@ViralBShah
Copy link
Member

Agree. We should just use scale. @dmbates Would you be able to make this change and create a pull request?

@dmbates
Copy link
Member Author

dmbates commented Apr 23, 2013

@ViralBShah I will make the change and create a pull request. Just to be clear, what I intend to do is to

  • add methods for scale corresponding to the methods for diagmm
  • add methods for scale! corresponding to the methods for diagmm!
  • deprecate diagmm in favor of scale and diagmm! in favor of scale!

I have a question about deprecating. When doing so do you remove the old method definitions? It appears so from the definition of the deprecate macro but I wanted to confirm this.

@ViralBShah
Copy link
Member

That's right - for deprecation, remove the old methods altogether.

@ViralBShah
Copy link
Member

Closed by #2918

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

3 participants