JuliaStats / PDMats.jl

Uniform Interface for positive definite matrices of various structures
Other
104 stars 43 forks source link

Don't export `dim` #169

Closed cscherrer closed 2 years ago

cscherrer commented 2 years ago

dim as a function name could have a wide variety of uses. If you ask any non-PDMats user what they think dim means, the answer is very unlikely to involve positive definite matrices.

The main problem with having this exported is that lots of user code using PDMats could come to have lots of dim references, so any attempt to define it in a more general sense could lead to problems. This is not something I'm currently running into; I just noticed that it's exported and it kind of seems like it shouldn't be.

ararslan commented 2 years ago

I'd argue that dim could be safely removed altogether, as it's redundant with both size and rank for positive definite matrices.

matbesancon commented 2 years ago

it's redundant with both size and rank for positive definite matrices

I'm confused by this part, one can have a rank-one n*n matrix, dim is just n regardless of the rank no?

devmotion commented 2 years ago

I think an advantage of dim is that it returns size(x, 1) AND ensures that it is equal to size(x, 2). In particular now that we started to extend the API to AbstractMatrix, this makes a difference. So I think it's rather dim(x::AbstractMatrix) = LinearAlgebra.checksquare(x).

So I think we could deprecate dim in favour of checksquare.

ararslan commented 2 years ago

one can have a rank-one n*n matrix, dim is just n regardless of the rank no?

That's true in general but I'm referring to the case covered by this package, i.e. positive definite matrices.