Opened 6 years ago
Closed 6 years ago
#16865 closed defect (fixed)
Dot2tex reverses Poset.show() upside down
Reported by:  jmantysalo  Owned by:  

Priority:  minor  Milestone:  sage6.7 
Component:  packages: optional  Keywords:  poset, dot2tex 
Cc:  ncohen, sagecombinat, vdelecroix  Merged in:  
Authors:  Nicolas M. Thiéry  Reviewers:  Nathann Cohen, Jori Mäntysalo 
Report Upstream:  N/A  Work issues:  
Branch:  08e9b8e (Commits)  Commit:  08e9b8e4e2109bec4ffb7d0b6807b5278495883c 
Dependencies:  Stopgaps: 
Description (last modified by )
Poset([[1,2],[[1,2]]]).top()
says 2
, so one expect
Poset([[1,2],[[1,2]]]).show()
showing to "2" on top. And so it happens without dot2tex. However, it shows "1" at top if dot2tex is installed. This was changed from version 6.1 to 6.2.
This ticket implements a rankdir
option for DiGraph.layout_acyclic
and sets a default value of 'up' for all implementations, which fixes the above inconsistency, while leaving the down
feature available to whoever would really care.
Change History (35)
comment:1 Changed 6 years ago by
 Description modified (diff)
 Milestone set to sage6.7
 Priority changed from trivial to minor
comment:2 Changed 6 years ago by
comment:3 Changed 6 years ago by
Unfortunately I do not know enought Sage internals to ba able to comment on this. I just reported this...
comment:4 Changed 6 years ago by
 Cc ncohen sagecombinat vdelecroix added
comment:5 Changed 6 years ago by
Sorry, LaTeX
output of graphs is combinat code. I don't maintain it.
comment:6 followup: ↓ 7 Changed 6 years ago by
I am not asking you to maintain it. I am asking your opinion on what should be done. Note that this is *not* about LaTeX
output.
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 6 years ago by
I am not asking you to maintain it. I am asking your opinion on what should be done.
It should be fixed. Poset([[1,2],[[1,2]]]).top()
says '2', so '2' should be on top. It apparently fails to do so when your package dot2tex
is installed (which is why I said 'latex output'), and I do not see exactly what there is to do except... fix it.
Or is the problem that graphviz/dot2tex can't plot the top element on top?
Nathann
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 6 years ago by
Replying to ncohen:
I am not asking you to maintain it. I am asking your opinion on what should be done.
It should be fixed.
Sure. That's not the question.
I have proposed two strategies for this in 2. Which one do you prefer. If we go for the first one, what's the right way for storing such a ploting option in the graph?
comment:9 in reply to: ↑ 8 ; followup: ↓ 12 Changed 6 years ago by
I have proposed two strategies for this in 2. Which one do you prefer. If we go for the first one, what's the right way for storing such a ploting option in the graph?
Storing a plotting option in the graph? Why on earth would you do that? O_o
Why can't you just make the default be to plot the top element on top, and only do something different if some guy calls my_poset.show(mess_up_top_and_bottom=True)
?
comment:10 followup: ↓ 11 Changed 6 years ago by
(I am not even sure that implementing an option to inverse top and bottom upon request is worth the time)
comment:11 in reply to: ↑ 10 Changed 6 years ago by
Replying to ncohen:
(I am not even sure that implementing an option to inverse top and bottom upon request is worth the time)
No, it isn't. One can use P.dual().show()
when needed for that.
Is it so that we can not have something like show(..., direction='down')
when we use dot2tex? (I.e. that would need modifying dot2tex code.)
comment:12 in reply to: ↑ 9 ; followup: ↓ 13 Changed 6 years ago by
Replying to ncohen:
I have proposed two strategies for this in 2. Which one do you prefer. If we go for the first one, what's the right way for storing such a ploting option in the graph?
Storing a plotting option in the graph? Why on earth would you do that?
O_o
So that when you create a hasse diagram, and later plot it, it remembers to plot it going up. But I can live without this feature.
Why can't you just make the default be to plot the top element on top, and only do something different if some guy calls
my_poset.show(mess_up_top_and_bottom=True)
?
That is not the question. The goal is to always plot the poset with top on top.
But to plot a poset we need to plot a digraph. So either it's ok if the acyclic layout of graphs always points up. Or we need to have an option (of the graph, or just of Graph.plot) to specify whether the acyclic layout goes up or down.
comment:13 in reply to: ↑ 12 Changed 6 years ago by
But to plot a poset we need to plot a digraph. So either it's ok if the acyclic layout of graphs always points up.
It is the default behaviour in Sage. If it is messed up when dot2tex gets installed then let *this* be fixed, and let us move on.
They chose that an arrow a>b
meant that "a is larger than b" and Posets
do the opposite. I do not think that it is worth more consideration than that.
Nathann
comment:14 Changed 6 years ago by
 Branch set to u/nthiery/graphs/dot2texrankdir16865
comment:15 Changed 6 years ago by
 Commit set to 502e62137a30c3bb8e2d03e84a23764f748f6a5e
 Status changed from new to needs_review
New commits:
502e621  16865: implement rankdir for Digraph.layout_acyclic, enforce a default value of 'up' for all implementations, and use it for posets

comment:16 Changed 6 years ago by
 Description modified (diff)
comment:17 followup: ↓ 19 Changed 6 years ago by
With your code, the value of 'rankdir' is dropped without a warning if have_dot2tex
is False
.
comment:18 Changed 6 years ago by
 Commit changed from 502e62137a30c3bb8e2d03e84a23764f748f6a5e to 22cd536e59262ed7bc025c61900d411117af391c
Branch pushed to git repo; I updated commit sha1. New commits:
22cd536  16865: pass down rankdir from layout_acyclic to layout_acyclic_dummy + fixed tests

comment:19 in reply to: ↑ 17 Changed 6 years ago by
Replying to ncohen:
With your code, the value of 'rankdir' is dropped without a warning if
have_dot2tex
isFalse
.
Oops, yes; and the tests in layout_acyclic
were using in fact layout_acyclic_dummy
... Fixed.
Luckily that would have been caught by the patchbot. But thanks for the early feedback!
comment:20 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:21 Changed 6 years ago by
Why needs work?
comment:22 followup: ↓ 25 Changed 6 years ago by
Sorry, I had not noticed that you had added a commit. Since you apparently "added a parameter" in 'graphviz_string' could you document it? If you do not want to document the same parameter in different places only because it is forwarded to some function later, could you say so explicitly in the docstring so that people can actually figure out that this 'rankdir' parameter exists?
comment:23 Changed 6 years ago by
 Commit changed from 22cd536e59262ed7bc025c61900d411117af391c to 95a223bd3635b7fce45bda3ddc77ab40fe43d5c9
Branch pushed to git repo; I updated commit sha1. New commits:
95a223b  16865: documentation of the rankdir option for graphviz_string

comment:24 Changed 6 years ago by
 Commit changed from 95a223bd3635b7fce45bda3ddc77ab40fe43d5c9 to 08e9b8e4e2109bec4ffb7d0b6807b5278495883c
Branch pushed to git repo; I updated commit sha1. New commits:
08e9b8e  16865: documentation of the rankdir option for graphviz_string: proofreading

comment:25 in reply to: ↑ 22 Changed 6 years ago by
Replying to ncohen:
Since you apparently "added a parameter" in 'graphviz_string' could you document it?
Ah, yes, thanks for catching this. Just an oversight. Given that the
default is not the same as for acyclic_layout
we really want to
document this option here. I used the occasion to clean a bit the
beginning of this docstring.
New commits:
08e9b8e  16865: documentation of the rankdir option for graphviz_string: proofreading

New commits:
08e9b8e  16865: documentation of the rankdir option for graphviz_string: proofreading

comment:26 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:27 followup: ↓ 28 Changed 6 years ago by
Jori? Could you try this patch? I do not have dot2tex installed on my machine, and these things cannot be uninstalled.
Nathann
comment:28 in reply to: ↑ 27 ; followup: ↓ 29 Changed 6 years ago by
Replying to ncohen:
Jori? Could you try this patch? I do not have dot2tex installed on my machine
Just tested quickly. Seems to work, but it takes a little longer to really test this.
and these things cannot be uninstalled.
That could be a reason for another ticket...
comment:29 in reply to: ↑ 28 Changed 6 years ago by
Just tested quickly. Seems to work, but it takes a little longer to really test this.
Well, please change the ticket's status when you are done, then.
That could be a reason for another ticket...
It is not a problem with 'dot2tex' only.
Nathann
comment:30 followup: ↓ 31 Changed 6 years ago by
 Description modified (diff)
I tested with some random posets and with empty and 1element poset. Seems to work.
But still I don't know Sage internals enought to be able to review this. Sorry for that.
comment:31 in reply to: ↑ 30 ; followup: ↓ 32 Changed 6 years ago by
 Reviewers set to Nathann Cohen
I tested with some random posets and with empty and 1element poset. Seems to work.
But still I don't know Sage internals enought to be able to review this. Sorry for that.
I asked you to test it because I cannot. I read the code, and I think that it can pass. If you are satisfied with the drawings (try all options) please change the ticket's status and add your name to the list of reviewers.
Nathann
comment:32 in reply to: ↑ 31 Changed 6 years ago by
 Reviewers changed from Nathann Cohen to Nathann Cohen, Jori Mäntysalo
 Status changed from needs_review to positive_review
Replying to ncohen:
If you are satisfied with the drawings (try all options) please change the ticket's status and add your name to the list of reviewers.
Tested. (Not all combinations, thought.) I think that this one is ready. For a note: layout='tree'
reverses the poset. But I doubt anyone would use it for posets.
comment:33 Changed 6 years ago by
 Status changed from positive_review to needs_work
Author name missing
comment:34 Changed 6 years ago by
 Status changed from needs_work to positive_review
comment:35 Changed 6 years ago by
 Branch changed from u/nthiery/graphs/dot2texrankdir16865 to 08e9b8e4e2109bec4ffb7d0b6807b5278495883c
 Resolution set to fixed
 Status changed from positive_review to closed
+1 to uniformly drawing posets with the top on top. This has annoyed me as well.
For LaTeX / PDF output, this is taken care of by the
backward
option ofset_latex_options
inPoset.hasse_diagram
(called byPoset._latex_
).For plots,
Poset.plot
callsGraph.plot
on the hasse diagram, and passes it by default thelayout=acyclic
option. The layout is then computed byGraph.layout_acyclic
which in turn either callsGraph.layout_acyclic_dummy
orGraph.layout_graphviz
ifdot2tex
is available.It turns out that
Graph.layout_acyclic_dummy
orients edges pointing up, while,Graph.layout_graphviz
uses the default behaviour of graphviz, namely to have edges pointing down. Hence the varying behaviour.Soo ... what's the cleanest way out?
One strategy:
backward
trick inset_latex_option
)layout_acyclic_dummy
andlayout_graphviz
honor this option.Another strategy would be to set as default for all graphs that the acyclic layout shall be going up. That's simpler. But not consistent with graphviz. It also also a larger change for the users (but only those using acyclic layout and having dot2tex installed).
What do you think?
Cheers,