Opened 5 years ago

Closed 5 years ago

#17449 closed defect (fixed)

deprecate/remove Graph.to_partition and Poset.to_graph

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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 sage-devel [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 sage-devel 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/sage-devel/TeIwE2L2MOQ

Change History (28)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/17449
  • Cc vdelecroix added
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to a8822900d5d9e428f4fd92d52ed9d00840ef61df

Branch pushed to git repo; I updated commit sha1. New commits:

a882290trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph

comment:3 follow-up: Changed 5 years ago by tscrim

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 5 years ago by vdelecroix

Hi Travis,

It was emphasized in the discussion on sage-devel 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 5 years ago by tscrim

As I stated, either make the docstring clear or, if you strongly believe that doc is not sufficient, rename the function.

comment:6 Changed 5 years ago by ncohen

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.

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 5 years ago by vdelecroix

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 5 years ago by ncohen

Yo Travis ! Could you answer our questions so that this ticket can move forward ?

Thanks,

Nathann

comment:9 follow-up: Changed 5 years ago by tscrim

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 5 years ago by ncohen

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 non-expert user who would miss .hase_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

Version 0, edited 5 years ago by ncohen (next)

comment:11 follow-up: Changed 5 years ago by tscrim

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 ; follow-up: Changed 5 years ago by ncohen

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 ; follow-up: Changed 5 years ago by tscrim

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 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.

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 ; follow-up: Changed 5 years ago by ncohen

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.

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 ; follow-up: Changed 5 years ago by tscrim

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 5 years ago by ncohen

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 5 years ago by ncohen

Travis ? It has been two days. Please help us solve this problem.

comment:18 follow-up: Changed 5 years ago by tscrim

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 ; follow-up: Changed 5 years ago by ncohen

  • 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 sage-devel thread.

Thanks,

Nathann

comment:20 in reply to: ↑ 19 Changed 5 years ago by tscrim

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 sage-devel 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 5 years ago by git

  • Commit changed from a8822900d5d9e428f4fd92d52ed9d00840ef61df to 4266c4499c8d358e68cc19e54f7370dbba5ac869

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4266c44trac #17449: deprecate/remove Graph.to_partition and Poset.to_graph

comment:22 Changed 5 years ago by git

  • Commit changed from 4266c4499c8d358e68cc19e54f7370dbba5ac869 to fc84f9d853236f76f720dc1ef4290468873d34f6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fc84f9dtrac #17449: deprecate/remove Graph.to_partition and Poset.to_graph

comment:23 Changed 5 years ago by ncohen

Could anybody turn this into a positive review ?

Nathann

comment:24 Changed 5 years ago by dimpase

  • Status changed from needs_review to positive_review

LGTM

comment:25 Changed 5 years ago by ncohen

Thanks Dima !

Nathann

comment:26 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:27 Changed 5 years ago by dimpase

  • Reviewers set to Dima Pasechnik, Travis Scrimshaw
  • Status changed from needs_work to positive_review

comment:28 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/17449 to fc84f9d853236f76f720dc1ef4290468873d34f6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.