Not clear to me that this is a more useful definition thanl += frac.

I'd argue that most people have no idea what the L value of a color is, but they know they want it 50% lighter.

FWIW and based solely on my limited own experience, I would be inclined to agree with@jklymak about the relative vs. absolute increment scheme.

The question here is not whether users understand L, but whether they expectbrighten_color(brighten_color(x, 0,2), 0.2) == brighten_color(x, 0.4) (as opposed to0.44, as you implement it).

Also thefloat is pointless here, as you convert it above

Given that the formula is explicitly in the docs, I hope they realize they are incrementing by 20% each time.

Color Brightening

I think you don't need slightly since the level of brightness or darkness is relative to the fraction. Please explain what the fraction is?

Its in the docstring which is linked, and should be obvious from the example...

don't think these all need to be on separate lines

What about parameterization? and do these floats need to be this long?

TestBrighten(object):test_colors= ('red','red','red','red','red', [0,.5,.9],'slategrey')test_brighten= (0,0.50,1.00,-0.50,-1.00,0.20,-0.20)known_brighten_result= ((1.0,0.0,0.0), (1.0,0.5,0.5),               (1.0,1.0,1.0), (0.5,0.0,0.0), (0.0,0.0,0.0),               (0.080000000000000071,0.59111111111111092,1.0),               (0.35097730430754981,0.40156862745098038,0.45215995059441105))@pytest.mark.parametrize('color, shade, expected',zip(test_colors,test_brighten,known_brighten_result))deftest_color_brightening(color,shade,expected):sc=mcolors.brighten_color(color,shade)assert_equal(sc,expected)

I didn't write the test, so this is a disinterested comment, but: I'm still not sure what the advantage of parameterization is? I don't find the above very easy to parse...

You don't have to explicitly loop through the cases and in this case there's no needed for the secondary helper function.

hmmm, well, there was no need for the helper fcn anyway. And

@pytest.mark.parametrize('color, shade, expected',                    zip(test_colors, test_brighten, known_brighten_result))

is not any shorter than writing

forcolor,brighten,expectedinzip(test_colors,test_brighten,known_brighten_result):

The latter has the benefit of being standard python, whereas the former requires knowing pytest decorators. I'm not adverse to change, but only if there is a compelling reason!

The two big wins from parameterization come from when you can stack more than 1 and get the outer-product of the input and because it lets each test fail independently.

I always forget about that, but the test failing independenty is the real reason to use parameterization becuase that's what makes it a true unit test. Plus in pytest you can add a list of lables for each paramterized set and so you can identify what the inputs are testing for.

I think we're missing the obvious case that this should just be written:

brighten=mcolors.brighten_colorassert_equal(brighten('red',0.0), (1.0,0.0,0.0))assert_equal(brighten('red',0.5), (1.0,0.5,0.5))assert_equal(brighten('red',1.0), (1.0,1.0,1.0))...

These aren't test parameters - they're the tests themselves, and the loop and helper functions just obscure the pairing between the results and the expected values

to 🚲 🏠 the name a bit, should this beadjust_color oradjust_tint or something like that? I almost left a comment after quickly skimming the code as "do we need to add a darken color too?" (and then I read the docstring and left this comment instead).

Does a function in the modulecolors really need to havecolor as part of its name?

adjust_lightness? Opens up to adjust saturation.

Seems foolish to use the non-vectorizedcolorsys functions when we already have a vectorizedrgb_to_hsv higher up in the file.

I know that HSV and HSL are different, but either we should write a vectorizedrgb_to_hsl for consistency, or just use the HSV one that's already here

This is outside what I'm willing to do for this PR.

If we could just use HSV, that'd be great, but my understanding is that to get white in HSV you need to change both S and V, whereas in HSL you can just change L. HSL seems the better colourspace for what we are doing here.

Just for context, this is an old PR that was sitting around for 2+ years, so I'm willing to put it through, but I'm not interested enough in getting the functionality that I'm willing to write a whole rgb_to_hsl conversion routine. If someone wants to put in a PR that does that, either before this one is accepted, or after, then that'd be swell. But I think the PR has merit even if it is not vectorized.

Are these both tuples? Justassert sc == expected.

I'm not convinced that this operation really makes a lot of sense, e.g. is going from L=0.1 to L=0.2 (frac=1) really "equivalent" from going to L=0.5 to L=1? If anything I think this should something like a probit or logit scale.

Seethis comment - I agree with you

I don’t feel strongly about this. Do you want black to stay black or go grey? I could go either way. I still prefer fractional change because I think that’s what most people would mean when they say to lighten an image. I’d you want the histogram to move right but black would still stay black.

We could also just forget any algorithm and just ask the user for the L value they want perhaps w an accompanying way to get L so they know where they are starting from.

I would indeed prefer a design where we expose a color class with methods such ascolor.hsl_replace(l=...),color.hsv_replace(s=...) (and allowing multiple kwargs of course, just leaving the ones not specified untouched).