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
KeyError raised by GraphLatex.dot2tex_picture when edge_labels=True #13624
Comments
This comment has been minimized.
This comment has been minimized.
comment:2
I managed to fix the bug with the following hack : diff --git a/src/dot2tex/dot2tex.py b/src/dot2tex/dot2tex.py
--- a/src/dot2tex/dot2tex.py
+++ b/src/dot2tex/dot2tex.py
@@ -1153,6 +1153,20 @@ To see what happened, run dot2tex with t
for name,item in usededges.items():
edge = item
+ break
hp,dp,wt = pp.texdims[name]
xmargin, ymargin = self.get_margins(edge)
labelcode = '<<<table border="0" cellborder="0" cellpadding="0">'\ but I still don't know what was this loop for... because the output is OK (with edge labels). |
comment:3
I just created an issue on the dot2tex google code project : |
This comment has been minimized.
This comment has been minimized.
comment:5
Hi Sébastien, I had not noticed this ticket. A quick and dirty workaround has been in the Sage-Combinat queue for some time. I did some further cleanup, but it's still nothing but a workaround. Shall we get it into Sage? If you think the patch could use more work, do you mind taking it over? Cheers, |
comment:6
While I was at it, I folded in a couple tests I had put in a later patch in the Sage-Combinat queue. |
This comment has been minimized.
This comment has been minimized.
comment:9
The change verb -> text in sage.misc.latex was breaking quite a few doctests. I made it a separate ticket: #14256. This one should not depend on it. On the other hand, I added three tiny things that were in a later patch in the queue:
|
comment:10
Please add your name as Author. |
Author: Nicolas M. Thiéry |
Changed keywords from dot2tex to dot2tex, graph drawing |
comment:12
Hi Nathann! Would you have time to review this one? Thanks! |
comment:13
This is what happens when I install graphviz
Nathann |
comment:14
Hi! Yes, the graphviz spkg is broken (and as far as I can remember has always been). But most linux distro have a graphviz package, so that's not too bad. The doc already says to install graphviz out of Sage. We should probably remove this broken spkg from "experimental" ... Cheers, |
comment:15
...... who the hell put that in the doc without removing the package ?... Nathann |
comment:16
Indeed. The only thing to do is update the doc to remove any mention of this graphviz package, and ask Jeroen in a ticket to remove it from experimental. And it will be quickly reviewed, I swear. Nathann |
comment:17
Replying to @nathanncohen:
I was already careful not to mention the spkg in the doc (but if you see how to improve the graphviz installation instructions, please go ahead in a separate ticket; I'll review it). As for removing the spkg, this is now: #14398. |
comment:18
Is written explicitly anywhere that there is no spkg and that the users should install it by themselves ? Nathann |
comment:19
should be
|
comment:20
(and while you're editing the patch, you included a http link which Sphinx does not understand as one) Nathann |
comment:21
5 doctests fail on my machine. The error message do not seem to say that there is something wrong with my install (dot or graphviz which I installed through my distro).
Nathann |
comment:22
Replying to @nathanncohen:
It's written that the soft can be downloaded from the graphviz website. This seems explicit enough to me, but if you want to improve that, go ahead (but I would not speak of spkg at all). |
comment:23
Where ? Nathann |
comment:42
Ils sont fous ces romains... Replying to @nthiery:
I think it is better not to update the old ticket #7004. Here is the link to the new spkg:
|
comment:43
I just added a patch which fixes the broken doctests seen by the patchbot. I also added a doctest that make sure the issue of this ticket is solved. I also fixed an # optional - dot2tex, graphviz which was not up to date. Finally I realize that some optional test are broken. Like this one that we can see in the doc of
|
comment:44
Replying to @seblabbe:
Thanks Sébastien! I looked at your reviewer patch, and it looks good to me. Let's see what the patchbot says. Back to needs review for the first patch.
+1 since the tests were already failing before this ticket. This is |
comment:45
For the record, current installation instructions for dot2tex are outdated (I tried the example from the description and it failed at first). With the current patch at #14382, the example from the description does not give errors. |
Adding a doctest to make sure the initial issue is fixed |
comment:46
Attachment: trac_13624-review-sl.patch.gz I just updated my patch which adds a doctest. Even if #14382 finally fix the problem, I think it could be merged anyway. I removed the part that was fixing dot2tex optional doctests errors. I will move this part in another patch that I am going to post on #14408 where similar issues were discussed. |
comment:50
Currently in
So how much of this ticket should we keep or should we just close as a "works-for-me"? |
comment:51
As I suggested in a previous comment, I think we could merge the patch Sébastien |
Commit: |
Branch: u/slabbe/13624 |
Changed author from Nicolas M. Thiéry to Sébastien Labbé |
Reviewer: Christian Stump |
Changed branch from u/slabbe/13624 to |
The following is ok :
but with
edges_labels=True
a KeyError is raised :The error is also raised by
dot2tex_picture
method:This is due to dot2tex currently not supporting '\verb' properly.
The attached patch works around this by replacing \verb by \text, and stripping out tricky characters like
^
and_
.CC: @stumpc5 @nathanncohen
Component: packages: optional
Keywords: dot2tex, graph drawing
Author: Sébastien Labbé
Branch/Commit:
2634500
Reviewer: Christian Stump
Issue created by migration from https://trac.sagemath.org/ticket/13624
The text was updated successfully, but these errors were encountered: