search

alashworth / test-issue-import

0 stars 0 forks source link

rename cov_matrix to spd_matrix #1

Open alashworth opened 5 years ago

alashworth commented 5 years ago

Issue by bob-carpenter Thursday Oct 03, 2013 at 05:43 GMT Originally opened as https://github.com/stan-dev/stan/issues/250

"spd" is for symmetric positive definite. The problem with "cov_matrix" is that they can be used for precision matrices, too, making the declaration look confusing.

This will also match the functions Marcus has added (but aren't quite yet through the pipe into the Stan language).

We should just deprecate cov_matrix with a warning.

We should also rename the Cholesky factors to match.

(Thanks to Ben for noticing the problem and Marcus for suggesting a better name.)

alashworth commented 5 years ago

Comment by jgabry Friday Mar 04, 2016 at 18:52 GMT

Is the plan still to deprecate cov_matrix in favor of spd_matrix? If so I'll add this to the v3 milestone (since it will break backwards compatibility) and keep it open. If not I'll close this.

alashworth commented 5 years ago

Comment by bob-carpenter Friday Mar 04, 2016 at 22:18 GMT

Let me bring it up on stan-dev.

On Mar 4, 2016, at 1:52 PM, Jonah Gabry notifications@github.com wrote:

Is the plan still to deprecate cov_matrix in favor of spd_matrix? If so I'll add this to the v3 milestone (since it will break backwards compatibility) and keep it open. If not I'll close this.

— Reply to this email directly or view it on GitHub.

alashworth commented 5 years ago

Comment by VMatthijs Thursday Dec 13, 2018 at 13:27 GMT

Is this still desired, or should we close the issue?

alashworth commented 5 years ago

Comment by bob-carpenter Thursday Dec 13, 2018 at 14:21 GMT

Yes, it's still desired.

"cov_matrix" should be deprecated and replaced with "spd_matrix".

"cholesky_factor_cov" should be deprecated and replaced with either just plain "cholesky_factor" or "cholesky_factor_spd" (the latter clarifies it's full rank).

I don't like that "cov" ("spd") is a prefix in one type and a suffix in another, but we should probably stick with this now unless people feel strongly that it should be rationalized going forward.

On Dec 13, 2018, at 8:27 AM, Matthijs Vákár notifications@github.com wrote:

Is this still desired, or should we close the issue?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.