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: sage-6.7
Component: packages: optional Keywords: poset, dot2tex
Cc: ncohen, sage-combinat, 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 jmantysalo)

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 jmantysalo

  • Description modified (diff)
  • Milestone set to sage-6.7
  • Priority changed from trivial to minor

comment:2 Changed 6 years ago by nthiery

+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 of set_latex_options in Poset.hasse_diagram (called by Poset._latex_).

For plots, Poset.plot calls Graph.plot on the hasse diagram, and passes it by default the layout=acyclic option. The layout is then computed by Graph.layout_acyclic which in turn either calls Graph.layout_acyclic_dummy or Graph.layout_graphviz if dot2tex 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:

  • have an option in graphs stating whether the preferred acyclic layout is going up or down (a generalization of the backward trick in set_latex_option)
  • make sure both layout_acyclic_dummy and layout_graphviz honor this option.
  • configure this option appropriately in the construction of the Hasse diagram.

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,

Nicolas

comment:3 Changed 6 years ago by jmantysalo

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 nthiery

  • Cc ncohen sage-combinat vdelecroix added

comment:5 Changed 6 years ago by ncohen

Sorry, LaTeX output of graphs is combinat code. I don't maintain it.

comment:6 follow-up: Changed 6 years ago by nthiery

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

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

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

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

(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 jmantysalo

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

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 ncohen

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 nthiery

  • Branch set to u/nthiery/graphs/dot2tex-rankdir-16865

comment:15 Changed 6 years ago by nthiery

  • Commit set to 502e62137a30c3bb8e2d03e84a23764f748f6a5e
  • Status changed from new to needs_review

New commits:

502e62116865: 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 nthiery

  • Description modified (diff)

comment:17 follow-up: Changed 6 years ago by ncohen

With your code, the value of 'rankdir' is dropped without a warning if have_dot2tex is False.

comment:18 Changed 6 years ago by git

  • Commit changed from 502e62137a30c3bb8e2d03e84a23764f748f6a5e to 22cd536e59262ed7bc025c61900d411117af391c

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

22cd53616865: pass down rankdir from layout_acyclic to layout_acyclic_dummy + fixed tests

comment:19 in reply to: ↑ 17 Changed 6 years ago by nthiery

Replying to ncohen:

With your code, the value of 'rankdir' is dropped without a warning if have_dot2tex is False.

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 ncohen

  • Status changed from needs_review to needs_work

comment:21 Changed 6 years ago by nthiery

Why needs work?

comment:22 follow-up: Changed 6 years ago by ncohen

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 git

  • Commit changed from 22cd536e59262ed7bc025c61900d411117af391c to 95a223bd3635b7fce45bda3ddc77ab40fe43d5c9

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

95a223b16865: documentation of the rankdir option for graphviz_string

comment:24 Changed 6 years ago by git

  • Commit changed from 95a223bd3635b7fce45bda3ddc77ab40fe43d5c9 to 08e9b8e4e2109bec4ffb7d0b6807b5278495883c

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

08e9b8e16865: documentation of the rankdir option for graphviz_string: proofreading

comment:25 in reply to: ↑ 22 Changed 6 years ago by nthiery

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:

08e9b8e16865: documentation of the rankdir option for graphviz_string: proofreading

New commits:

08e9b8e16865: documentation of the rankdir option for graphviz_string: proofreading

comment:26 Changed 6 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:27 follow-up: Changed 6 years ago by ncohen

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

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 ncohen

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 follow-up: Changed 6 years ago by jmantysalo

  • Description modified (diff)

I tested with some random posets and with empty and 1-element 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 ; follow-up: Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen

I tested with some random posets and with empty and 1-element 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 jmantysalo

  • 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 vbraun

  • Status changed from positive_review to needs_work

Author name missing

comment:34 Changed 6 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Status changed from needs_work to positive_review

comment:35 Changed 6 years ago by vbraun

  • Branch changed from u/nthiery/graphs/dot2tex-rankdir-16865 to 08e9b8e4e2109bec4ffb7d0b6807b5278495883c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.