Opened 6 years ago

Closed 6 years ago

#14177 closed enhancement (fixed)

More uniform handling of color_by_labels for graph plot, plot3d, graphviz, and reference fix

Reported by: nthiery Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.8
Component: graph theory Keywords:
Cc: sage-combinat Merged in: sage-5.8.beta2
Authors: Nicolas M. Thiéry Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

Back in #7004 I introduced some more flexibility for the color_by_labels option for graphviz, in order to allow for specifying a dictionary or function mapping labels to color. The attached patch extends this feature to plot and plot3d, by having them call _color_by_label uniformly. It also implements vertex labels for 3D plots.

In the long run, one would want to factor out the edge/vertex/color/labels option handling to guarantee uniformness; but that's for another ticket.

Apply: trac_14177-graph-color_by_labels-nt.patch, trac_14177-badref.patch

Attachments (2)

trac_14177-graph-color_by_labels-nt.patch (9.9 KB) - added by nthiery 6 years ago.
trac_14177-badref.patch (1.4 KB) - added by ncohen 6 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 years ago by nthiery

  • Description modified (diff)
  • Status changed from new to needs_review

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

Oh ! I just saw that on my rss feeds: could you add my name as a cc next time ? I don't automatically get emails when something gets created in the graph section.

Nathann

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

  • Status changed from needs_review to needs_work

HMmmmm.... Really ? :-P

``color_by_label`` - a boolean (default: False)
...
sage: p = G.plot(edge_labels=True, color_by_label={'a':'yellow', 'b':'purple'}); p 

This being said, what would you think of a color_by_label argument that can be set to False (all edges receive the same color), to a dictionary like you do right now, or to True, in which case you can give each label a specific color (with g.edge_labels() and rainbow) ?

Nathann

comment:4 in reply to: ↑ 2 ; follow-up: Changed 6 years ago by nthiery

Replying to ncohen:

Oh ! I just saw that on my rss feeds: could you add my name as a cc next time ? I don't automatically get emails when something gets created in the graph section.

Sure. That being said, maybe you want to ask the trac maintainer to add you to the cc list for the graph section!

Cheers,

comment:5 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by nthiery

Replying to ncohen:

HMmmmm.... Really ? :-P

``color_by_label`` - a boolean (default: False)
...
sage: p = G.plot(edge_labels=True, color_by_label={'a':'yellow', 'b':'purple'}); p 

Yes, really! Just take a slightly larger extract :-)

        - ``color_by_label`` - a boolean (default: False)
           whether to color each edge with a different color according
           to its label; this can also take as value a function or
           dictionary mapping labels to colors; this option is
           incompatible with ``edge_color`` and ``edge_colors``.

I agree it's not perfect, as you may at first glance miss the information that the value can be something else than a boolean. But I did not find a better formulation without being heavy in the formaluation. Better suggestions welcome. Maybe something like:

        - ``color_by_label`` - a boolean, dictionary or function (default: False)
           whether to color each edge with a different color according
           to its label; the value can be a function or
           dictionary mapping labels to colors; this option is
           incompatible with ``edge_color`` and ``edge_colors``.

This being said, what would you think of a color_by_label argument that can be set to False (all edges receive the same color), to a dictionary like you do right now, or to True, in which case you can give each label a specific color (with g.edge_labels() and rainbow) ?

This is exactly what this is doing currently, isn't it?

Cheers,

Nicolas

comment:6 in reply to: ↑ 4 Changed 6 years ago by ncohen

Sure. That being said, maybe you want to ask the trac maintainer to add you to the cc list for the graph section!

Is that different fom being in the "owned by" field ? O_o

I will ask then.

Nathann

comment:7 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by ncohen

I agree it's not perfect, as you may at first glance miss the information that the value can be something else than a boolean. But I did not find a better formulation without being heavy in the formaluation. Better suggestions welcome. Maybe something like:

        - ``color_by_label`` - a boolean, dictionary or function (default: False)
           whether to color each edge with a different color according
           to its label; the value can be a function or
           dictionary mapping labels to colors; this option is
           incompatible with ``edge_color`` and ``edge_colors``.

Well, saying that this variable is a boolean when it is not is clearly not an option. What you just wrote looks nice !

This is exactly what this is doing currently, isn't it?

Oh. Cool. I looked at the patch and did not see anything related with rainbow, nor any loop, so I assumed it did not do it, but now I realise that it was the former behaviour :-)

Nathann

Changed 6 years ago by nthiery

comment:8 in reply to: ↑ 7 Changed 6 years ago by nthiery

Replying to ncohen:

Well, saying that this variable is a boolean when it is not is clearly not an option. What you just wrote looks nice !

The updated patch uses yet another quick variant. By the way, I added appropriate doc to the option descriptions in graph_plot.py.

Oh. Cool. I looked at the patch and did not see anything related with rainbow, nor any loop, so I assumed it did not do it, but now I realise that it was the former behaviour :-)

:-)

Cheers,

Nicolas

comment:9 Changed 6 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:10 Changed 6 years ago by ncohen

Ok, looks good and passes all tests ! It took some time because I am not very familiar with this part of the code, and also because there seems to be something wrong with the html documentation right now ^^;

Thanks for this patch !!!

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

Ahem... Would you have anything against adding this to your ticket ? It's not worth creating another one, and I'm pretty sure to forget it if I do not do it soon ^^;

Nathann

Changed 6 years ago by ncohen

comment:12 Changed 6 years ago by ncohen

(Samuel reported this problem : the page mentionned is not updated anymore, and anyway the LP section disappeared from the book, as it did not add anything interesting to it)

Nathann

comment:13 in reply to: ↑ 11 Changed 6 years ago by nthiery

  • Status changed from needs_review to positive_review

Replying to ncohen:

Ahem... Would you have anything against adding this to your ticket ? It's not worth creating another one, and I'm pretty sure to forget it if I do not do it soon ^^;

Did not you say something about small independent patches some day?

:-)

Yeah, sure I don't mind. So off to positive review.

comment:14 Changed 6 years ago by nthiery

  • Description modified (diff)
  • Summary changed from More uniform handling of color_by_labels for graph plot, plot3d, graphviz to More uniform handling of color_by_labels for graph plot, plot3d, graphviz, and reference fix

comment:15 Changed 6 years ago by nthiery

And thanks for the quick review!

comment:16 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.8.beta2
  • Resolution set to fixed
  • Reviewers changed from Nathann Cohen? to Nathann Cohen
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.