Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore the "backward" option for graph dot2tex #32438

Closed
tscrim opened this issue Aug 30, 2021 · 18 comments
Closed

Restore the "backward" option for graph dot2tex #32438

tscrim opened this issue Aug 30, 2021 · 18 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Aug 30, 2021

The change made on #31381 is not compatible with the previous behavior of backward=True for the edge options with dot2tex rendering of graphs. In particular, this was seen when viewing KR crystals such as

view(crystals.KirillovReshetikhin(['C',3,1], 3,1))

We will add back in the backward option, which supports different behavior than dir='back':

  • backward=True will be a layout-only indication that a particular arrow should be oriented in the opposite direction compared to other arrows.
  • dir='back' will have the arrows pointing in the backwards direction.

This also reverts the change in #31719 to get the previous behavior.

CC: @sagetrac-sage-combinat @seblabbe @fchapoton @anneschilling @dwbump @nthiery @bsalisbury1

Component: graphics

Keywords: dot2tex

Author: Travis Scrimshaw

Branch/Commit: 7e2b1de

Reviewer: Sébastien Labbé

Issue created by migration from https://trac.sagemath.org/ticket/32438

@tscrim tscrim added this to the sage-9.5 milestone Aug 30, 2021
@tscrim
Copy link
Collaborator Author

tscrim commented Aug 30, 2021

Commit: 1da92af

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator Author

tscrim commented Aug 30, 2021

New commits:

1da92afReverting to "backward" option for dot2tex graph layout.

@tscrim
Copy link
Collaborator Author

tscrim commented Aug 30, 2021

Branch: public/graphs/backwards_option-32438

@tscrim tscrim changed the title Restore the "backwards" option for graph dot2tex Restore the "backward" option for graph dot2tex Aug 30, 2021
@tscrim
Copy link
Collaborator Author

tscrim commented Sep 8, 2021

comment:2

Green bot (essentially).

@fchapoton
Copy link
Contributor

comment:3

I would prefer that Sébastien gives a positive review, if possible.

@seblabbe
Copy link
Contributor

seblabbe commented Sep 8, 2021

comment:4

Yes, please, let me take a look. I was overloaded by the "rentrée". Next days will be better, I check this tomorrow on Thursday.

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 8, 2021

comment:5

No problem. There isn't a big rush on this. I would appreciate it if this got into 9.5. :)

@seblabbe
Copy link
Contributor

seblabbe commented Sep 9, 2021

comment:6

I still haven't had the opportunity the try the branch. My last update of sage broke and I need to take a look. But looking at the proposed branch, I have one comment:

the change

+        - ``"backward"`` (boolean)

needs documentation. Maybe for an English speaker it is clear, but to me, it is difficult to understand the difference between dir='back' and backward=True. That difference needs to be explained in words here. Secondly, it should explicit what it does (that is, u,v=v,u and etc.). Also, it should be explained that it is something that will work only when prog='dot' and not for the other Graphviz programs like neato, fdp, circo, etc.

This is why I still think it is kind of a hack but I don't disagree to put it back.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2021

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

13a57edMerge branch 'public/graphs/backwards_option-32438' of git://trac.sagemath.org/sage into public/graphs/backwards_option-32438
0c38588Adding more description to the backward=True example.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2021

Changed commit from 1da92af to 0c38588

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 10, 2021

comment:8

I added a bit more to the example to indicate more clearly what changes, which is in line with the rest of the documentation for that section. Everything within that the code does works for all such programs (although it may or may not have an impact, but that is irrelevant).

It is definitely not a hack as the layout order matters (for at least one program), and we have mathematical information attached to the digraph that would be wrong if we change the arrows.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2021

Changed commit from 0c38588 to 7e2b1de

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2021

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

7e2b1de32438: added documentation for option 'backward'

@seblabbe
Copy link
Contributor

comment:11

I added some documentation to the 'backward' option. I give a positive review to the commits made before mine. I let Travis change the status to positive review if he agrees with my commit.

@seblabbe
Copy link
Contributor

Reviewer: Sébastien Labbé

@tscrim
Copy link
Collaborator Author

tscrim commented Sep 30, 2021

comment:13

Let's get this in.

@vbraun
Copy link
Member

vbraun commented Oct 9, 2021

Changed branch from public/graphs/backwards_option-32438 to 7e2b1de

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants