Sage: Ticket #17449: deprecate/remove Graph.to_partition and Poset.to_graph
https://trac.sagemath.org/ticket/17449
<p>
As discussed on sage-devel [1], the two function <code>Graph.to_partition</code> and <code>Poset.to_graph</code> do not have a very meaningful name.
</p>
<p>
As <code>Poset.to_graph</code> can be obtained from the clearer alternatives <code>Graph(Poset.hasse_diagram())</code> or <code>Poset.hasse_diagram().to_undirected()</code> it is not a problem to remove it. On the other hand, the discussion on sage-devel seemed to indicate that <code>Graph.to_partition</code> should be renamed rather than removed.
</p>
<p>
Also, as it appeared that people were more interested in the size of connected components than in the actual <code>Partition</code> object, the new function will only return a list of integers.
</p>
<p>
Nathann
</p>
<p>
[1] <a class="ext-link" href="https://groups.google.com/forum/#!topic/sage-devel/TeIwE2L2MOQ"><span class="icon"></span>https://groups.google.com/forum/#!topic/sage-devel/TeIwE2L2MOQ</a>
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/17449
Trac 1.1.6ncohenSat, 06 Dec 2014 05:00:53 GMTstatus changed; cc, branch set
https://trac.sagemath.org/ticket/17449#comment:1
https://trac.sagemath.org/ticket/17449#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>cc</strong>
<em>vdelecroix</em> added
</li>
<li><strong>branch</strong>
set to <em>u/ncohen/17449</em>
</li>
</ul>
TicketgitSat, 06 Dec 2014 05:01:42 GMTcommit set
https://trac.sagemath.org/ticket/17449#comment:2
https://trac.sagemath.org/ticket/17449#comment:2
<ul>
<li><strong>commit</strong>
set to <em>a8822900d5d9e428f4fd92d52ed9d00840ef61df</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=a8822900d5d9e428f4fd92d52ed9d00840ef61df"><span class="icon"></span>a882290</a></td><td><code>trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph</code>
</td></tr></table>
TickettscrimSat, 06 Dec 2014 08:23:18 GMT
https://trac.sagemath.org/ticket/17449#comment:3
https://trac.sagemath.org/ticket/17449#comment:3
<p>
I don't first think of posets as directed graphs, so having a method <code>to_graph</code> says this conversion exists and the fact that you have to go through <code>hasse_diagram()</code> hints that we should leave the method in there. If you feel it's not clear, then add more to the function's documentation and teach people to look at the doc if they can't discern what the function does from it's name and output.
</p>
TicketvdelecroixSat, 06 Dec 2014 12:40:50 GMT
https://trac.sagemath.org/ticket/17449#comment:4
https://trac.sagemath.org/ticket/17449#comment:4
<p>
Hi Travis,
</p>
<p>
It was emphasized in the discussion on sage-devel that <code>to_graph</code> is ambiguous. What about <code>coverings_digraph</code> (or <code>to_coverings_digraph</code>) as an alias for <code>hasse_diagram</code> as it was proposed by Jori ?
</p>
<p>
Vincent
</p>
TickettscrimSun, 07 Dec 2014 17:28:41 GMT
https://trac.sagemath.org/ticket/17449#comment:5
https://trac.sagemath.org/ticket/17449#comment:5
<p>
As I stated, either make the docstring clear or, if you strongly believe that doc is not sufficient, rename the function.
</p>
TicketncohenMon, 08 Dec 2014 07:03:35 GMT
https://trac.sagemath.org/ticket/17449#comment:6
https://trac.sagemath.org/ticket/17449#comment:6
<p>
Travis, I wanted to hear your opinion on this but you did not answer my latest post. What do you believe is wrong with removing this function ? As I explained on sage-devel, it is incorrect to have a method named "to_graph" as it clearly is not specific enough.
</p>
<p>
All we can do is rename it, but to what ? I do not see any way to rename it that would not make it totally useless, as I said on the ticket. Please tell us what you think of this point, as I would like to see this matter settled in the next days.
</p>
<p>
Thanks,
</p>
<p>
Nathann
</p>
TicketvdelecroixMon, 08 Dec 2014 07:10:42 GMT
https://trac.sagemath.org/ticket/17449#comment:7
https://trac.sagemath.org/ticket/17449#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17449#comment:3" title="Comment 3">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I don't first think of posets as <strong>directed</strong> graphs, so having a method <code>to_graph</code> says this conversion exists
</p>
</blockquote>
<p>
It does not
</p>
<pre class="wiki">sage: P = posets.ChainPoset(3)
sage: P.to_graph()
Graph on 3 vertices
sage: P.to_graph().is_directed()
False
</pre>
TicketncohenThu, 11 Dec 2014 10:04:48 GMT
https://trac.sagemath.org/ticket/17449#comment:8
https://trac.sagemath.org/ticket/17449#comment:8
<p>
Yo Travis ! Could you answer our questions so that this ticket can move forward ?
</p>
<p>
Thanks,
</p>
<p>
Nathann
</p>
TickettscrimThu, 11 Dec 2014 16:51:53 GMT
https://trac.sagemath.org/ticket/17449#comment:9
https://trac.sagemath.org/ticket/17449#comment:9
<p>
I've stated it before, I believe putting things into the documentation is sufficient and having a method <code>to_graph</code> says there is a "natural" conversion to a graph through considering a Poset as a directed graph. So if you want to rename it, don't expect me to put it the work to find a "better" name. It doesn't hurt anyone by being there (with good documentation).
</p>
TicketncohenThu, 11 Dec 2014 17:07:53 GMT
https://trac.sagemath.org/ticket/17449#comment:10
https://trac.sagemath.org/ticket/17449#comment:10
<blockquote class="citation">
<p>
I've stated it before, I believe putting things into the documentation is sufficient and having a method <code>to_graph</code> says there is a "natural" conversion to a graph through considering a Poset as a directed graph. So if you want to rename it, don't expect me to put it the work to find a "better" name. It doesn't hurt anyone by being there (with good documentation).
</p>
</blockquote>
<p>
Travis, please be fair with us. You believed yourself until Vincent's comment that the function returned a digraph while it returns a graph. You see how confusing it can be. Also it is incorrect to say that there is a "natural" conversion to graph: there is none, because there are many ways to obtain a graph/digraph. None is canonical.
</p>
<p>
Furthermore, we want to remove <code>to_graph</code> because properly named methods already exist:
</p>
<pre class="wiki">sage: p = Poset()
sage: p.incomparability_graph()
Incomparability graph on 0 vertices
sage: p.hasse_diagram()
Digraph on 0 vertices
</pre><p>
Why would we keep a misnamed function when we already have two which do the job right ?
</p>
<p>
We can already see that a Poset can be converted into a graph from the list of Poset methods. And a non-expert user who would miss <code>.hasse_diagram</code> (because it does not contain the word 'graph') will not miss <code>incomparability_graph</code>.
</p>
<p>
With all this, I am sorry that I do not understand why you would want to keep this <code>to_graph</code> function.
</p>
<p>
Nathann
</p>
TickettscrimThu, 11 Dec 2014 17:29:10 GMT
https://trac.sagemath.org/ticket/17449#comment:11
https://trac.sagemath.org/ticket/17449#comment:11
<p>
Then be fair to me and don't put words into my mouth. I didn't mean <code>to_graph</code> returns a directed graph, I meant we couldn't feed a poset into Graph, much less to go through Hasse diagram. Also I didn't say canonical, I said natural. The function is not misnamed, it converts the poset into a graph. If you thing the name is ambiguous (which is what I understand your argument to be), then my counter is to put more into the documentation. If you think this is not sufficient, then you propose an alternative name.
</p>
TicketncohenThu, 11 Dec 2014 17:38:12 GMT
https://trac.sagemath.org/ticket/17449#comment:12
https://trac.sagemath.org/ticket/17449#comment:12
<p>
Hello,
</p>
<blockquote class="citation">
<p>
If you thing the name is ambiguous (which is what I understand your argument to be), then my counter is to put more into the documentation. If you think this is not sufficient, then you propose an alternative name.
</p>
</blockquote>
<p>
Sorry, but I still do not understand why you believe that this function can be useful to anyone. If it is only to advertise that Posets can be turned into graphs, isn't <code>.incomparability_graph</code> already doing the job ?
</p>
<p>
The only new name I can think of is <code>.undirected_hasse_diagram</code>, but with <code>.hasse_diagram()</code> around we hardly need it.
</p>
<p>
I do not understand why you fight so hard for this. I do not see it.
</p>
<p>
Nathann
</p>
TickettscrimThu, 11 Dec 2014 17:50:33 GMT
https://trac.sagemath.org/ticket/17449#comment:13
https://trac.sagemath.org/ticket/17449#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17449#comment:12" title="Comment 12">ncohen</a>:
</p>
<blockquote class="citation">
<p>
Sorry, but I still do not understand why you believe that this function can be useful to anyone. If it is only to advertise that Posets can be turned into graphs, isn't <code>.incomparability_graph</code> already doing the job ?
</p>
</blockquote>
<p>
It is a different graph than (in)comparability graph, but the <code>to_graph</code> is a natural construction.
</p>
<blockquote class="citation">
<p>
The only new name I can think of is <code>.undirected_hasse_diagram</code>, but with <code>.hasse_diagram()</code> around we hardly need it.
</p>
</blockquote>
<p>
Why? Because it's a one-line function? Then we should get rid of more one-line functions too. Just because you don't see the use, doesn't mean there isn't one.
</p>
<blockquote class="citation">
<p>
I do not understand why you fight so hard for this. I do not see it.
</p>
</blockquote>
<p>
I don't understand your vendetta against this function either. It doesn't hurt anyone as it clearly documents what it does.
</p>
TicketncohenThu, 11 Dec 2014 17:59:49 GMT
https://trac.sagemath.org/ticket/17449#comment:14
https://trac.sagemath.org/ticket/17449#comment:14
<blockquote class="citation">
<p>
Why? Because it's a one-line function? Then we should get rid of more one-line functions too. Just because you don't see the use, doesn't mean there isn't one.
</p>
</blockquote>
<p>
Can you tell me its use ? It would help me to understand why you think that it is useful.
</p>
<p>
Nathann
</p>
TickettscrimThu, 11 Dec 2014 18:06:24 GMT
https://trac.sagemath.org/ticket/17449#comment:15
https://trac.sagemath.org/ticket/17449#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17449#comment:14" title="Comment 14">ncohen</a>:
</p>
<blockquote class="citation">
<p>
Can you tell me its use ? It would help me to understand why you think that it is useful.
</p>
</blockquote>
<p>
It takes a poset and returns a graph based upon its cover relations. It is a natural construction (that does not exist as a method elsewhere) on posets that a user could try. Also methods state what operations can be performed on a particular object.
</p>
TicketncohenFri, 12 Dec 2014 03:38:31 GMT
https://trac.sagemath.org/ticket/17449#comment:16
https://trac.sagemath.org/ticket/17449#comment:16
<blockquote class="citation">
<p>
It takes a poset and returns a graph based upon its cover relations. It is a natural construction (that does not exist as a method elsewhere) on posets that a user could try.
</p>
</blockquote>
<p>
I still do not understand. So you say that this function is useful because it returns that construction ? But we already have <code>hasse_diagram</code> for this, which does roughly the same and that users can try too. It is actually better, in the sense that it is faster to compute and that it contains more information.
</p>
<p>
Nathann
</p>
TicketncohenSun, 14 Dec 2014 01:50:07 GMT
https://trac.sagemath.org/ticket/17449#comment:17
https://trac.sagemath.org/ticket/17449#comment:17
<p>
Travis ? It has been two days. Please help us solve this problem.
</p>
TickettscrimSun, 14 Dec 2014 17:17:23 GMT
https://trac.sagemath.org/ticket/17449#comment:18
https://trac.sagemath.org/ticket/17449#comment:18
<p>
Yet <code>hasse_diagram</code> does not do the same thing as that returns the directed graph. Again, the function is documented (and we have interactive documentation). It's not hurting anyone, so there's no problem; just leave it be.
</p>
TicketncohenSun, 14 Dec 2014 17:54:19 GMTcc changed
https://trac.sagemath.org/ticket/17449#comment:19
https://trac.sagemath.org/ticket/17449#comment:19
<ul>
<li><strong>cc</strong>
<em>dimpase</em> added
</li>
</ul>
<blockquote class="citation">
<p>
Again, the function is documented (and we have interactive documentation). It's not hurting anyone, so there's no problem; just leave it be.
</p>
</blockquote>
<p>
I do believe that it does harm by being there. It is not a function that should ever be used in Sage or by anyone else, as other ways exist to get the same result and because its name is unclear: we could very well decide to make it return a different graph later, the function would still be correct as ".to_graph()" would still turn the Poset into "a graph". I think that knowledgeable people here (like Dima) call that 'semantics'.
</p>
<p>
While I understand that you want users to easily find out that there exists a way to get a graph from a <code>Poset</code> by looking at the list of methods, I also believe that ".incomparability_graph" does the job and that users will find it. Those who know what they are looking for will find <code>.hasse_diagram</code> quickly enough.
</p>
<p>
Somehow, I think that it all boils down to not say "the integer" but "an integer" when "the integer" is not uniquely determined. It would be nice if Sage had this level of strictness unless it really goes in the way of readability.
</p>
<p>
Would you be willing, despite everything, to let us procede with the removal of this function ? The considerations above matter to me, and also to some extent to the other persons who concurred on the sage-devel thread.
</p>
<p>
Thanks,
</p>
<p>
Nathann
</p>
TickettscrimSun, 14 Dec 2014 18:09:01 GMT
https://trac.sagemath.org/ticket/17449#comment:20
https://trac.sagemath.org/ticket/17449#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17449#comment:19" title="Comment 19">ncohen</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Again, the function is documented (and we have interactive documentation). It's not hurting anyone, so there's no problem; just leave it be.
</p>
</blockquote>
<p>
I do believe that it does harm by being there. It is not a function that should ever be used in Sage or by anyone else, as other ways exist to get the same result and because its name is unclear: we could very well decide to make it return a different graph later, the function would still be correct as ".to_graph()" would still turn the Poset into "a graph". I think that knowledgeable people here (like Dima) call that 'semantics'.
</p>
</blockquote>
<p>
Then rename the method. Here's a possibility <code>cover_relations_graph</code>.
</p>
<blockquote class="citation">
<p>
While I understand that you want users to easily find out that there exists a way to get a graph from a <code>Poset</code> by looking at the list of methods, I also believe that ".incomparability_graph" does the job and that users will find it. Those who know what they are looking for will find <code>.hasse_diagram</code> quickly enough.
</p>
</blockquote>
<p>
<code>incomparability_graph</code> does not do the same thing. The <code>to_graph</code> only returns a graph by the cover relations, and it's a bad assumption that typical users will know what to do because they have not been raising hell over posets and graphs in Sage.
</p>
<blockquote class="citation">
<p>
Would you be willing, despite everything, to let us procede with the removal of this function ? The considerations above matter to me, and also to some extent to the other persons who concurred on the sage-devel thread.
</p>
</blockquote>
<p>
In short, no. I strongly believe in not removing functionality, and since you think something needs to change, then you can change the name.
</p>
TicketgitMon, 15 Dec 2014 01:19:11 GMTcommit changed
https://trac.sagemath.org/ticket/17449#comment:21
https://trac.sagemath.org/ticket/17449#comment:21
<ul>
<li><strong>commit</strong>
changed from <em>a8822900d5d9e428f4fd92d52ed9d00840ef61df</em> to <em>4266c4499c8d358e68cc19e54f7370dbba5ac869</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=4266c4499c8d358e68cc19e54f7370dbba5ac869"><span class="icon"></span>4266c44</a></td><td><code>trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph</code>
</td></tr></table>
TicketgitMon, 15 Dec 2014 01:27:16 GMTcommit changed
https://trac.sagemath.org/ticket/17449#comment:22
https://trac.sagemath.org/ticket/17449#comment:22
<ul>
<li><strong>commit</strong>
changed from <em>4266c4499c8d358e68cc19e54f7370dbba5ac869</em> to <em>fc84f9d853236f76f720dc1ef4290468873d34f6</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=fc84f9d853236f76f720dc1ef4290468873d34f6"><span class="icon"></span>fc84f9d</a></td><td><code>trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph</code>
</td></tr></table>
TicketncohenFri, 19 Dec 2014 05:21:54 GMT
https://trac.sagemath.org/ticket/17449#comment:23
https://trac.sagemath.org/ticket/17449#comment:23
<p>
Could anybody turn this into a positive review ?
</p>
<p>
Nathann
</p>
TicketdimpaseFri, 19 Dec 2014 15:06:20 GMTstatus changed
https://trac.sagemath.org/ticket/17449#comment:24
https://trac.sagemath.org/ticket/17449#comment:24
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
LGTM
</p>
TicketncohenFri, 19 Dec 2014 15:48:58 GMT
https://trac.sagemath.org/ticket/17449#comment:25
https://trac.sagemath.org/ticket/17449#comment:25
<p>
Thanks Dima !
</p>
<p>
Nathann
</p>
TicketvbraunSun, 21 Dec 2014 10:56:05 GMTstatus changed
https://trac.sagemath.org/ticket/17449#comment:26
https://trac.sagemath.org/ticket/17449#comment:26
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Reviewer name
</p>
TicketdimpaseSun, 21 Dec 2014 12:41:21 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/17449#comment:27
https://trac.sagemath.org/ticket/17449#comment:27
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Dima Pasechnik, Travis Scrimshaw</em>
</li>
</ul>
TicketvbraunFri, 02 Jan 2015 15:46:37 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/17449#comment:28
https://trac.sagemath.org/ticket/17449#comment:28
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>u/ncohen/17449</em> to <em>fc84f9d853236f76f720dc1ef4290468873d34f6</em>
</li>
</ul>
Ticket