Opened 8 years ago

Closed 8 years ago

#12904 closed defect (fixed)

colors from rainbow don't work in 3d plots

Reported by: kcrisman Owned by: jason, was
Priority: major Milestone: sage-5.1
Component: graphics Keywords:
Cc: Merged in: sage-5.1.beta5
Authors: André Apitzsch Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11383 Stopgaps:

Description

----------------------------------------------------------------------
| Sage Version 4.8, Release Date: 2012-01-20                         |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
sage: R = rainbow(1)
sage: R[0]
'#ff0000'
sage: P = plot(x,color=R[0])
sage: P  # all is well
sage: P = plot3d(x,(0,1),(0,1),color=R[0])
ValueError: unknown color '#ff0000'
sage: P = plot3d(x,(0,1),(0,1),color=Color(R[0]))
sage: P # all is well

Attachments (1)

trac_12904.patch (3.7 KB) - added by aapitzsch 8 years ago.

Download all attachments as: .zip

Change History (9)

Changed 8 years ago by aapitzsch

comment:1 Changed 8 years ago by aapitzsch

  • Authors set to André Apitzsch
  • Dependencies set to #11383
  • Status changed from new to needs_review

The necessary change to solve the problem is

  • sage/plot/plot3d/texture.py

    a b  
    211212        return info.rgb()
    212213    elif isinstance(info, str):
    213214        try:
    214             return colors[info]
     215            return Color(info)
    215216        except KeyError:

comment:2 follow-up: Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

This is great diagnosis and a nice terse doctest. I've looked through this a fair amount and the only obvious problem (other than the doctest failure below) I can find is that

sage: timeit('colors.has_key("blue")')
625 loops, best of 3: 205 ns per loop
sage: timeit('"blue" in colors')
625 loops, best of 3: 230 ns per loop

and more or less this is true all the time. I don't know if there is any documentation on which is uniformly faster? In some sense the second seems more "Pythonic" but OO syntax is nice too...

I also wonder why the switch to the new Python error raising but not the new formatting :) but that is really totally unimportant.

But did you run tests?

sage -t  "devel/sage-main/sage/plot/plot3d/texture.py"      
**********************************************************************
File "/Users/.../sage-5.0.beta14/devel/sage-main/sage/plot/plot3d/texture.py", line 19:
    sage: [t for t in G.texture_set() if t.color == colors.red] # we should have two red textures
Expected:
    [Texture(texture..., red, ff0000), Texture(texture..., red, ff0000)]
Got:
    []
**********************************************************************
File "/Users/.../sage-5.0.beta14/devel/sage-main/sage/plot/plot3d/texture.py", line 21:
    sage: [t for t in G.texture_set() if t.color == colors.yellow] # ...and one yellow
Expected:
    [Texture(texture..., yellow, ffff00)]
Got:
    []
**********************************************************************
1 items had failures:
   2 of  11 in __main__.example_0
***Test Failed*** 2 failures.
For whitespace errors, see the file /Users/karl-dietercrisman/.sage//tmp/texture_43616.py
	 [10.8 s]

I have a feeling that this is also indicative:

sage: colors['blue']
RGB color (0.0, 0.0, 1.0)
sage: Color('blue')
RGB color (0.0, 0.0, 1.0)
sage: type(colors['blue'])
<class 'sage.plot.colors.Color'>
sage: type(Color('blue'))
<class 'sage.plot.colors.Color'>
sage: colors['blue'] == Color('blue')
False

Anyway, I'm a little surprised because colors has a constructor from Color... but needs work.

comment:3 in reply to: ↑ 2 Changed 8 years ago by aapitzsch

  • Status changed from needs_work to needs_review

Replying to kcrisman:

This is great diagnosis and a nice terse doctest. I've looked through this a fair amount and the only obvious problem (other than the doctest failure below) I can find is that

sage: timeit('colors.has_key("blue")')
625 loops, best of 3: 205 ns per loop
sage: timeit('"blue" in colors')
625 loops, best of 3: 230 ns per loop

and more or less this is true all the time. I don't know if there is any documentation on which is uniformly faster? In some sense the second seems more "Pythonic" but OO syntax is nice too...

I also wonder why the switch to the new Python error raising but not the new formatting :) but that is really totally unimportant.

What do you mean by new formatting?

has_key is deprecated, see docs.python, that's why I replaced it. BTW, I got

sage: timeit('colors.has_key("blue")') 
625 loops, best of 3: 186 ns per loop
sage: timeit('"blue" in colors')      
625 loops, best of 3: 187 ns per loop

But did you run tests?

sage -t  "devel/sage-main/sage/plot/plot3d/texture.py"      
**********************************************************************
File "/Users/.../sage-5.0.beta14/devel/sage-main/sage/plot/plot3d/texture.py", line 19:
    sage: [t for t in G.texture_set() if t.color == colors.red] # we should have two red textures
Expected:
    [Texture(texture..., red, ff0000), Texture(texture..., red, ff0000)]
Got:
    []
**********************************************************************
File "/Users/.../sage-5.0.beta14/devel/sage-main/sage/plot/plot3d/texture.py", line 21:
    sage: [t for t in G.texture_set() if t.color == colors.yellow] # ...and one yellow
Expected:
    [Texture(texture..., yellow, ffff00)]
Got:
    []
**********************************************************************
1 items had failures:
   2 of  11 in __main__.example_0
***Test Failed*** 2 failures.
For whitespace errors, see the file /Users/karl-dietercrisman/.sage//tmp/texture_43616.py
	 [10.8 s]

You have to apply the patch for #11383 first, see dependencies.

I'm setting this back to needs review because of the reasons mentioned above.

comment:4 Changed 8 years ago by kcrisman

Oh, I had no idea about the dependency. Love the link about the in, very useful. By new formatting I mean things like this Python doc.

I don't have time to do finish this off right now but I'm sure that this will be fine once I apply #11383. Good work.

comment:5 Changed 8 years ago by kcrisman

  • Status changed from needs_review to positive_review

I don't see any other problems here. Doesn't mean they aren't there, but this seems good.

comment:6 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-pending

comment:7 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.1

comment:8 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.1.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.