Opened 8 years ago

Last modified 5 years ago

#12414 needs_info enhancement

system-wide default color

Reported by: jason Owned by: jason, was
Priority: major Milestone: sage-6.4
Component: graphics Keywords:
Cc: kcrisman Merged in:
Authors: Jason Grout Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jason)

It would be useful to have a system-wide default plotting color. For example, when embedding in latex, you want all the default line colors that are now blue to be black instead.

This patch implements a sage.plot.colors.DEFAULTCOLOR variable that is used, instead of specifying blue in every object.

This patch avoids touching objects that don't have the default color blue so that it won't be very controversial. Cleaning up the graphics in Sage is a much bigger project...

apply: trac-12414-defaultcolor.patch

Attachments (1)

trac-12414-defaultcolor.patch (11.5 KB) - added by jason 8 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by jason

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

comment:2 Changed 8 years ago by jason

  • Status changed from needs_review to needs_work

some doctests are failing...

comment:3 Changed 8 years ago by jason

  • Status changed from needs_work to needs_review

Fixed the failing doctests.

comment:4 Changed 8 years ago by jason

  • Status changed from needs_review to needs_work

Found a few more failing doctests...

comment:5 Changed 8 years ago by jason

  • Status changed from needs_work to needs_review

And now all doctests in plot/ pass...

comment:6 Changed 8 years ago by kcrisman

Ok, this all makes sense to me (and a great idea!) and works well, apparently.

I do except the 3D issues. Can you explain to me the rationale for the changes in texture.py? On a related note, I do feel like we're changing end-user use with requiring the .rgb() business.

I'm not saying they're bad changes, but I don't see any particular reason (other than perhaps making doctests pass, which isn't quite sufficient by itself). Just need explanation/justification.

Also, what formats should work? I tried

sage: sage.plot.colors.DEFAULTCOLOR='black' 
sage: sage.plot.colors.DEFAULTCOLOR=(0,0,1)

and they worked. What about hsv or something? (Not that I know how we'd distinguish between them.) Or hex? Just wondering.

comment:7 Changed 8 years ago by jason

The changes to the 3d plot parse_color are to make parse_color more consistent and also to get it to use the default color if the color is None. Before, it returned a tuple if you passed in a Color, but it returned a Color if you passed in a string---not very consistent. I made it so that it always returns a tuple, and it returns the default color if called with nothing (that's the call to to_mpl_color).

Any format supported by any color= option should be supported. Strings, hex strings, rgb colors, color names, etc.

comment:8 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work

I see. In that case, you forgot to change the documentation!

       Parses the color.
    
       It transforms a valid color string into a color object and a color
       object into an RBG tuple of length 3. Otherwise, it multiplies the
       info by the base color.
    

On the other issue - before:

sage: [t.color for t in G.texture_set()]
[RGB color (1.0, 0.0, 0.0), RGB color (1.0, 0.0, 0.0), RGB color (1.0, 1.0, 0.0)]

after:

sage: [t.color for t in G.texture_set()]
[(1.0, 0.0, 0.0), (1.0, 0.0, 0.0), (1.0, 1.0, 0.0)]

This really won't break anything in the initialization of Texture where this is used?


Also, I just found a doctest typo, unnoticed because of the fact that Texture pretty much takes any input without complaining. You didn't do this, but we should fix it here.

        sage: Texture(ambiant=0.7)
        Texture(texture..., 6666ff)

comment:9 follow-up: Changed 8 years ago by jason

  • Status changed from needs_work to needs_review

I'm waffling about whether I should back out the texture changes (I don't have time now to dive deeply into the 3d plotting). What do you think? I can just put an extra test in there that returns the default color if no color is passed in.

I've attached a patch which takes care of the typo and docs.

Changed 8 years ago by jason

comment:10 in reply to: ↑ 9 Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

I'm waffling about whether I should back out the texture changes (I don't have time now to dive deeply into the 3d plotting). What do you think? I can just put an extra test in there that returns the default color if no color is passed in.

I think that is the easier solution for now. I assume that most people wouldn't care, but I think there was some method to the way it's returned, so that the same parsing function could be used in a few different contexts. I don't know whether parse_color is used outside of the initialization of Texture, but I think it would be unfortunate for attributes of Textures to change format without discussion somewhere by someone who uses them.

If you do change that back, please give the new patch a different name, because then we have this other fix still on Trac for easy reference later.

I've attached a patch which takes care of the typo and docs.

Except there was a typo there from before this ticket which I just noticed. Aargh!

It transforms a valid color description into an RBG tuple of 

What is RBG, kcrisman? Silly... This would be needed no matter how you deal with parse_color.

comment:11 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_info

Just needing info on how it will proceed.

comment:12 Changed 8 years ago by jason

I'll put up a new patch later; no time now...

comment:13 Changed 8 years ago by jason

Getting back to this...where are we? Should I back out the texture part?

comment:14 Changed 8 years ago by kcrisman

I guess, except see #12904 which has positive review and makes a change to parse_color that might impact this.

comment:15 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:16 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:17 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:18 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.