Opened 4 years ago

Last modified 4 years ago

#18046 needs_work enhancement

Graphs with multiedges and latex

Reported by: jclaas Owned by:
Priority: major Milestone: sage-6.6
Component: graph theory Keywords: graph-theory, multiedges, latex, sd66
Cc: ncohen, nthiery, tscrim, aschilling Merged in:
Authors: Jacob Laas Reviewers:
Report Upstream: N/A Work issues:
Branch: u/jclaas/graphs_with_multiedges_and_latex (Commits) Commit: d2ebe228caee2da94ecee214bf7a8f52004228be
Dependencies: Stopgaps:

Description (last modified by jclaas)

It was discovered that the LaTeX module for graphs raises the NotImplementedError? for graphs containing multiple edges (multiedges=True). The intention of this ticket is to incorporate the fixes needed to typeset a graph with latex.

Change History (10)

comment:1 Changed 4 years ago by jclaas

  • Authors set to Jacob Laas
  • Branch set to u/jclaas/graphs_with_multiedges_and_latex
  • Cc ncohen nthiery tscrim aschilling added
  • Component changed from PLEASE CHANGE to graph theory
  • Description modified (diff)
  • Keywords graph-theory multiedges latex added
  • Status changed from new to needs_info
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 4 years ago by jclaas

I should note that I've already incorporated most of the necessary fixes into a local copy of sage/graphs/graph_latex.py and will soon post here a patch for the file, so that I can get some feedback..

comment:3 Changed 4 years ago by git

  • Commit set to b025f2ad103c5596edde303e6bcf983ecea4d0e2

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

b025f2aall features from local hack have been added, plus a warning about edge customizations

comment:4 Changed 4 years ago by jclaas

I've just pushed a commit containing updated code so that graphs with multiedges can be tex-ified, but there's a particular case that still causes issue: when a set of multiedges contain labels, their customizations do not match up correctly.

My first guess was that this due to the nature of using a dict_of_dicts to track all edge/vertex properties, which python then gets the order of hashable items mixed up.

See below for a test case illustrating the issue:

mygraph = DiGraph(multiedges=True)
mygraph.add_edge('A','B','thinnest')
mygraph.add_edge('A','B','blah')
mygraph.add_edge('A','B','thickest')
mygraph.add_edge('A','C','blah')
mygraph.add_edge('B','C','blah')
mygraph.add_edge('C','B','blah')
mygraph.set_latex_options(
    units='in',
    graphic_size=(6,6),
    edge_thickness=0.01,
    edge_labels=True,
    edge_thicknesses={('A','B'):[0.01,0.025,0.05]},
    )
print latex(mygraph)
view(latex(mygraph))

I've specified a list of thicknesses for the multiedges 'A' -> 'B'. Their labels were defined in a particular order, and the thicknesses are meant to match this order, but they don't.. any ideas?

comment:5 Changed 4 years ago by jclaas

  • Status changed from needs_info to needs_review

comment:6 Changed 4 years ago by git

  • Commit changed from b025f2ad103c5596edde303e6bcf983ecea4d0e2 to b3721b4fd42959a87ee415643a44776d2e8d83c2

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

b3721b4replaced new variables with long names, replaced stopgap with NotImplementedError, fixed typo in internal type check for edge customizations

comment:7 Changed 4 years ago by ncohen

  • Keywords sd66 added
  • Status changed from needs_review to needs_work

Hello,

Several other remarks:

  • These days I have been told that we should use isinstance(x,list) instead of type(x) in [list]. Note that the following works:

sage: isinstance([],(list,dict)) True sage: isinstance({},(list,dict)) True sage: isinstance(5,(list,dict)) True

  • Now, this file looks rather messy and you apparently tried to get your code to 'fit in the local style'. It seems that they define a variable named number_types that you could use.
  • et -> no short name please.
  • This line contains both and and or. Could you add parentheses where they belong ?

if self._graph.is_directed() or self._graph.allows_multiple_edges() and not loop:

  • Could you write the proper documentation (in the INPUT section of the docstring) for the parameter edge_thickness in case of multiple edges?
  • Your error message is misleading: it is not that multiple customization of multiedges is impossible, it is that only thickness can be set. At least, this is what your condition tests.
  • What is the difference between -> and post in TikZ? Why did you make that change?
  • Is there any reason why you call self._graph.edge_label(edge[0],edge[1])? That value should already be equal to e[2], shouldn't it?

Nathann

comment:8 Changed 4 years ago by jclaas

Thanks for your feedback. Sorry for missing some of the style rules, but some of the issues you raise are actually left over from previous work. Regardless, I've tried to fix them the best I can.

Also, I'm happy to report that I've gotten custom edge_thicknesses to work. It turned out that I just had to reverse the value of multiedge_index. I didn't dive too deep to find out why, as there's something like 27k lines of code I might have to search through.

  • These days I have been told that we should use isinstance(x,list) instead of type(x) in [list].

Fixed.

  • Now, this file looks rather messy and you apparently tried to get your code to 'fit in the local style'. It seems that they define a variable named number_types that you could use.

Fixed.

  • et -> no short name please.

Fixed.

  • This line contains both and and or. Could you add parentheses where they belong ?

Fixed.

  • Could you write the proper documentation (in the INPUT section of the docstring) for the parameter edge_thickness in case of multiple edges?

Fixed.

  • Your error message is misleading: it is not that multiple customization of multiedges is impossible, it is that only thickness can be set. At least, this is what your condition tests.

I've edited the message to read "Edge-specific colors are not available for multiedges.", so that it only refers to the case of edge_colors, which I wasn't up to the task of fixing immediately. The color issue seems like a lot of work, because each vertex/edge color is specified via unique \definecolor{}{rgb}{} latex definitions, instead of just passing colors as strings. From what I can tell from the documentation (though I don't understand French), tkz-graph understands colors as strings, so it seems like the entire set of \definecolor{}{rgb}{} definitions are unnecessary. Perhaps one should open a ticket just for updating this feature? I could see an update to this being more useful for my own purposes, but it also just seems better to just have colors specified within each edge's tex-string.

  • What is the difference between -> and post in TikZ? Why did you make that change?

I don't know what post is, but I replaced it with -> because post doesn't respect line widths and it's not in the most recent documentation (https://www.ctan.org/pkg/tkz-graph).

  • Is there any reason why you call self._graph.edge_label(edge[0],edge[1])? That value should already be equal to e[2], shouldn't it?

Hmm that wasn't my doing, but I did find out that attempting to refer to e[2] absolutely doesn't maintain the order in which the edge labels are called, which I find very bizarre. Leaving it as-is will at least ensure that my fix for edge_thicknesses works properly.

comment:9 Changed 4 years ago by git

  • Commit changed from b3721b4fd42959a87ee415643a44776d2e8d83c2 to d2ebe228caee2da94ecee214bf7a8f52004228be

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

d2ebe22- fixed edge_thicknesses and many styling issues

comment:10 Changed 4 years ago by ncohen

Helloooooooooooo,

Several coments:

  • Could you add to the doctests an example of a graph plot which uses the new feature you impplemented?
  • if isinstance(x, list) and (not isinstance(x[0], number_types) or not x[0] >= 0.0):

This code only tests if the *first* element of the list is a positive. You should probably replace x[0] >= 0.0 with all(xx>=0 for xx in x)

  • The previous test appears so often that it would be better to define a is_positive_number=lambda x:isinstance(x, number_types) and x >= 0.0 and use it everywhere.
  • In this block of code
    reverse_index = len(edge_thickness) - (multiedge_index+1)
    edge_thickness = edge_thickness[reverse_index]
    
    You should probably use the following Python trick
    sage: a=[1,2,3,4,5,6]
    sage: a[-1]
    6
    sage: a[-4]
    3
    sage: a[-5]
    2
    sage: a[-6]
    1
    

Short of this the code looks good. Thanks,

Nathann

Note: See TracTickets for help on using tickets.