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

Rename GraphLasso to GraphicalLasso to avoid ambiguity. #9993

Closed
zfrenchee opened this Issue Oct 24, 2017 · 14 comments

Comments

Projects
None yet
6 participants
@zfrenchee
Contributor

zfrenchee commented Oct 24, 2017

Graphical Lasso is a method for sparse inverse covariance estimation.

Graph Lasso is a method for sparse regression where the features lie on a graph.

These are different methods. Sklearn implements Graphical Lasso but not Graph Lasso, however I believe it wrongly calls Graphical Lasso GraphLasso, which causes some ambiguity. After a very quick search, it seems like the abiguity starts with this library (might be incorrect about this).

@agramfort

This comment has been minimized.

Member

agramfort commented Oct 24, 2017

@amueller

This comment has been minimized.

Member

amueller commented Oct 24, 2017

ugh... but we don't want to promote a wrong name, as people pick up our naming schemes.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 24, 2017

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 24, 2017

@agramfort

This comment has been minimized.

Member

agramfort commented Oct 25, 2017

@GaelVaroquaux

This comment has been minimized.

Member

GaelVaroquaux commented Oct 25, 2017

I am -1 on this. We'll never get the namings wrong, and I don't think that it really helps our users to change names for purity reasons. As mentioned above, there is probably not big ambiguity.

I'll plead guilty to this error, by the way. I am willing to take blame for it forever if it may help users :).

This is a weak -1: I don't think that the project will collapse or that the users will lead a revolution if we change this. It's just that such changes are cost on both sides (users and developers), so they should be done only if they really bring strong value.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 25, 2017

@zfrenchee

This comment has been minimized.

Contributor

zfrenchee commented Oct 25, 2017

Before we finalize this decision, I'd like to explain why I made the issue just a little more:

  • When I google "Graph Lasso Python" looking for a python implementation of Graph Lasso (not Graphical Lasso) all I can find has to do with Graphical Lasso because of this naming decision.
  • It may be that this misnaming is percolating out from this library, as @amueller suggests is possible.
  • I was interested in possible contributing an implementation of graph-lasso to this library, but renaming would be a necessary first step. (Though since there isn't too much interest in adding related, more popular methods (#9967) I'm not sure this idea would get off the ground...)
@GaelVaroquaux

This comment has been minimized.

Member

GaelVaroquaux commented Oct 25, 2017

@zfrenchee : I understand the logic, and I think that it has a lot of sense.

I would think that an actual graph lasso would probably be out of the scope of the library, so I guess that the last point is not crucial.

I would also worry about the poor naming (blame me) percolating out of the library. I think that it would be very using to submit a PR mentioning the confusion in the docstrings and the docs.

Given how confusing this is, we could go for introducing a GraphicalLasso object and having a very very very long deprecation time on the GraphLasso. That might be a way of bring benefice to all actors: people using GraphLasso just get a warning that points them to GraphicalLasso, but their code doesn't break (or the old tutorials that they are looking at still work). New people directly jump to GraphicalLasso.

Two action points above: a) docs, b) very soft deprecation. What are people's thoughts on both?

@zfrenchee

This comment has been minimized.

Contributor

zfrenchee commented Oct 25, 2017

@GaelVaroquaux I think your plan is a very good one. For now, adding a new GraphicalLasso object (which does the same as the current GraphLasso object), updating the docs to point to GraphicalLasso first instead of GraphLasso, and putting a message in the docs / docstrings of GraphLasso makes sense to me, as well as maybe throwing a warning when people use GraphLasso saying "this will eventually be deprecated".

My personal belief is that this mistake should be corrected in the long term, but the timeline could be... years, if we wanted. We could revisit the idea of deprecating GraphLasso much later.

What do others think?

@agramfort

This comment has been minimized.

Member

agramfort commented Oct 25, 2017

@amueller

This comment has been minimized.

Member

amueller commented Oct 25, 2017

I'm happy with a long deprecation cycle and docs for now.

@artiemq

This comment has been minimized.

Contributor

artiemq commented Oct 27, 2017

If nobody works on it I'd like to handle this issue.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment