Skip to content
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

improve docstring and add test fot to_rgb(<float>) #2610

Merged
merged 1 commit into from Nov 30, 2013

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Nov 22, 2013

Even if the docstring was saying it, emphasis that to_rgb/to_rgba do not
accept float as input, but string representation of a float.

As one might be tempted to had float->gray conversion add failing test
in case the functionality is added.

Side fixes :
change tuple([value]*3) to (value,)*3 for speed

In [18]: %timeit  tuple([0.4]*3)
1000000 loops, best of 3: 505 ns per loop

In [19]: %timeit  (0.4,)*3
10000000 loops, best of 3: 23.5 ns per loop

In [20]:  (0.4,)*3 == tuple([0.4]*3)
Out[20]: True

Should close #2609

Will post the result of the failing test if one modify to_rgb/to_rgba to accept float for info.

@Carreau
Copy link
Contributor Author

Carreau commented Nov 22, 2013

If one patch like that for example :

diff --git a/lib/matplotlib/colors.py b/lib/matplotlib/colors.py
index a49bc92..d429e62 100644
--- a/lib/matplotlib/colors.py
+++ b/lib/matplotlib/colors.py
@@ -301,7 +301,9 @@ class ColorConverter(object):
                     % (str(arg),))

         try:
-            if cbook.is_string_like(arg):
+            if type(arg) in (int,float):
+                return (arg,)*3
+            elif cbook.is_string_like(arg):
                 argl = arg.lower()
                 color = self.colors.get(argl, None)
                 if color is None:

Test should fail like that :

$ python tests.py matplotlib.tests.test_colors
........F
======================================================================
FAIL: `to_rgb`/`to_rgba` should not accept float; only string representation of float.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.5/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/nose-1.3.0-py2.7.egg/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/matthiasbussonnier/matplotlib/lib/matplotlib/tests/test_colors.py", line 212, in test_colors_no_float
    assert_raises(ValueError, gray_from_float_rgb)
AssertionError: ValueError not raised

----------------------------------------------------------------------
Ran 9 tests in 0.371s

FAILED (failures=1)

@mdboom
Copy link
Member

mdboom commented Nov 22, 2013

Looks like it just needs some PEP8 fixes.

@Carreau
Copy link
Contributor Author

Carreau commented Nov 22, 2013

Travis should be happy now.

$ python tests.py matplotlib.tests.test_coding_standards
.
----------------------------------------------------------------------
Ran 1 test in 14.457s

OK

return mcolors.colorConverter.to_rgb(0.4)

def gray_from_float_rgba():
return mcolors.colorConverter.to_rgb(0.4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_rgba

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you mean rgb -> rgba :-)

Even if the docstring was saing it, emphasis that to_rgb/to_rgba do not
accept float as input, but string representation of a float.

As one might be tempted to had float->gray conversion add failing test
in case the functionality is added.

Side fixes :
    change tuple([value]*3) to (value,)*3 for speed

In [18]: %timeit  tuple([0.4]*3)
1000000 loops, best of 3: 505 ns per loop

In [19]: %timeit  (0.4,)*3
10000000 loops, best of 3: 23.5 ns per loop

In [20]:  (0.4,)*3 == tuple([0.4]*3)
Out[20]: True

Closes matplotlibgh-2609
@Carreau
Copy link
Contributor Author

Carreau commented Nov 24, 2013

Fixed, squashed and rebased on master.

efiring added a commit that referenced this pull request Nov 30, 2013
improve docstring and add test fot to_rgb(<float>)
@efiring efiring merged commit f123e30 into matplotlib:master Nov 30, 2013
@Carreau Carreau deleted the close-2609 branch December 5, 2013 08:43
@Carreau
Copy link
Contributor Author

Carreau commented Dec 5, 2013

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_rgb(float) or to_rgb(str(flot))
3 participants