Opened 4 years ago
Closed 4 years ago
#17449 closed defect (fixed)
deprecate/remove Graph.to_partition and Poset.to_graph
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  graph theory  Keywords:  
Cc:  vdelecroix, dimpase  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Dima Pasechnik, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  fc84f9d (Commits)  Commit:  fc84f9d853236f76f720dc1ef4290468873d34f6 
Dependencies:  Stopgaps: 
Description
As discussed on sagedevel [1], the two function Graph.to_partition
and Poset.to_graph
do not have a very meaningful name.
As Poset.to_graph
can be obtained from the clearer alternatives Graph(Poset.hasse_diagram())
or Poset.hasse_diagram().to_undirected()
it is not a problem to remove it. On the other hand, the discussion on sagedevel seemed to indicate that Graph.to_partition
should be renamed rather than removed.
Also, as it appeared that people were more interested in the size of connected components than in the actual Partition
object, the new function will only return a list of integers.
Nathann
[1] https://groups.google.com/forum/#!topic/sagedevel/TeIwE2L2MOQ
Change History (28)
comment:1 Changed 4 years ago by
 Branch set to u/ncohen/17449
 Cc vdelecroix added
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
 Commit set to a8822900d5d9e428f4fd92d52ed9d00840ef61df
comment:3 followup: ↓ 7 Changed 4 years ago by
I don't first think of posets as directed graphs, so having a method to_graph
says this conversion exists and the fact that you have to go through hasse_diagram()
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.
comment:4 Changed 4 years ago by
Hi Travis,
It was emphasized in the discussion on sagedevel that to_graph
is ambiguous. What about coverings_digraph
(or to_coverings_digraph
) as an alias for hasse_diagram
as it was proposed by Jori ?
Vincent
comment:5 Changed 4 years ago by
As I stated, either make the docstring clear or, if you strongly believe that doc is not sufficient, rename the function.
comment:6 Changed 4 years ago by
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 sagedevel, it is incorrect to have a method named "to_graph" as it clearly is not specific enough.
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.
Thanks,
Nathann
comment:7 in reply to: ↑ 3 Changed 4 years ago by
Replying to tscrim:
I don't first think of posets as directed graphs, so having a method
to_graph
says this conversion exists
It does not
sage: P = posets.ChainPoset(3) sage: P.to_graph() Graph on 3 vertices sage: P.to_graph().is_directed() False
comment:8 Changed 4 years ago by
Yo Travis ! Could you answer our questions so that this ticket can move forward ?
Thanks,
Nathann
comment:9 followup: ↓ 10 Changed 4 years ago by
I've stated it before, I believe putting things into the documentation is sufficient and having a method to_graph
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).
comment:10 in reply to: ↑ 9 Changed 4 years ago by
I've stated it before, I believe putting things into the documentation is sufficient and having a method
to_graph
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).
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.
Furthermore, we want to remove to_graph
because properly named methods already exist:
sage: p = Poset() sage: p.incomparability_graph() Incomparability graph on 0 vertices sage: p.hasse_diagram() Digraph on 0 vertices
Why would we keep a misnamed function when we already have two which do the job right ?
We can already see that a Poset can be converted into a graph from the list of Poset methods. And a nonexpert user who would miss .hasse_diagram
(because it does not contain the word 'graph') will not miss incomparability_graph
.
With all this, I am sorry that I do not understand why you would want to keep this to_graph
function.
Nathann
comment:11 followup: ↓ 12 Changed 4 years ago by
Then be fair to me and don't put words into my mouth. I didn't mean to_graph
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.
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 4 years ago by
Hello,
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.
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 .incomparability_graph
already doing the job ?
The only new name I can think of is .undirected_hasse_diagram
, but with .hasse_diagram()
around we hardly need it.
I do not understand why you fight so hard for this. I do not see it.
Nathann
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 4 years ago by
Replying to ncohen:
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
.incomparability_graph
already doing the job ?
It is a different graph than (in)comparability graph, but the to_graph
is a natural construction.
The only new name I can think of is
.undirected_hasse_diagram
, but with.hasse_diagram()
around we hardly need it.
Why? Because it's a oneline function? Then we should get rid of more oneline functions too. Just because you don't see the use, doesn't mean there isn't one.
I do not understand why you fight so hard for this. I do not see it.
I don't understand your vendetta against this function either. It doesn't hurt anyone as it clearly documents what it does.
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 4 years ago by
Why? Because it's a oneline function? Then we should get rid of more oneline functions too. Just because you don't see the use, doesn't mean there isn't one.
Can you tell me its use ? It would help me to understand why you think that it is useful.
Nathann
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 4 years ago by
Replying to ncohen:
Can you tell me its use ? It would help me to understand why you think that it is useful.
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.
comment:16 in reply to: ↑ 15 Changed 4 years ago by
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.
I still do not understand. So you say that this function is useful because it returns that construction ? But we already have hasse_diagram
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.
Nathann
comment:17 Changed 4 years ago by
Travis ? It has been two days. Please help us solve this problem.
comment:18 followup: ↓ 19 Changed 4 years ago by
Yet hasse_diagram
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.
comment:19 in reply to: ↑ 18 ; followup: ↓ 20 Changed 4 years ago by
 Cc dimpase added
Again, the function is documented (and we have interactive documentation). It's not hurting anyone, so there's no problem; just leave it be.
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'.
While I understand that you want users to easily find out that there exists a way to get a graph from a Poset
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 .hasse_diagram
quickly enough.
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.
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 sagedevel thread.
Thanks,
Nathann
comment:20 in reply to: ↑ 19 Changed 4 years ago by
Replying to ncohen:
Again, the function is documented (and we have interactive documentation). It's not hurting anyone, so there's no problem; just leave it be.
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'.
Then rename the method. Here's a possibility cover_relations_graph
.
While I understand that you want users to easily find out that there exists a way to get a graph from a
Poset
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.hasse_diagram
quickly enough.
incomparability_graph
does not do the same thing. The to_graph
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.
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 sagedevel thread.
In short, no. I strongly believe in not removing functionality, and since you think something needs to change, then you can change the name.
comment:21 Changed 4 years ago by
 Commit changed from a8822900d5d9e428f4fd92d52ed9d00840ef61df to 4266c4499c8d358e68cc19e54f7370dbba5ac869
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4266c44  trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph

comment:22 Changed 4 years ago by
 Commit changed from 4266c4499c8d358e68cc19e54f7370dbba5ac869 to fc84f9d853236f76f720dc1ef4290468873d34f6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fc84f9d  trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph

comment:23 Changed 4 years ago by
Could anybody turn this into a positive review ?
Nathann
comment:25 Changed 4 years ago by
Thanks Dima !
Nathann
comment:27 Changed 4 years ago by
 Reviewers set to Dima Pasechnik, Travis Scrimshaw
 Status changed from needs_work to positive_review
comment:28 Changed 4 years ago by
 Branch changed from u/ncohen/17449 to fc84f9d853236f76f720dc1ef4290468873d34f6
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph