5
\$\begingroup\$

So I have the following code, that takes an image and modifies the RGB channels one at a time. It then creates a list of 9 images with the various modified channels.

The Red channel is modified by 0.1, then by 0.5, then by 0.9. This is then applied to the Green channel then the Blue Channel.

Resulting in a list of 9 images with the same modifications, but to different channels. The code works, but I just feel like there is a lot of repetition, and this really bugs me.

#%%

from PIL import Image

# Set the file and open it
file = "Final/charlie.png"

#Create an empty list to append the images to
images = []

#Image processing function, depending on the counter
def image_processing0(a):
    if count == 0 or count == 3 or count == 6:
        c = int(a * 0.1)
        return c
    elif count == 1 or count == 4 or count == 7:
        c = int(a * 0.5)
        return c
    elif count == 2 or count == 5 or count == 8:
        c = int(a * 0.9)
        return c

#Set the count to 0
count = 0

#Create PixelMap
while count <9:
#Creat Pic at the start of each loop
    pic = Image.open(file)
#Convert the image to RGB, so we dont have to deal with the alpha channel
    pic = pic.convert('RGB')
    
#Get the RGB value for every pixel
    for x in range(pic.size[0]):
            for y in range(pic.size[1]):

                #Split the RGB value into variables r, g and b
                r,g,b = pic.getpixel((x,y))
                
#Check the count, use logic to appy the processing to the correct channel               
                if count <3:
                    r = image_processing0(r)
                    pic.putpixel((x,y),(r,g,b))
                elif count < 6:
                    g = image_processing0(g)
                    pic.putpixel((x,y),(r,g,b))
                elif count < 9:
                    b = image_processing0(b)
                    pic.putpixel((x,y),(r,g,b))

#Add the images to the list                    
    images.append(pic)
#Increment the counter    
    count+=1  

Both my logic statement, and my function seem a bit repetitive:

 #---------------------------------------------------
    #Image processing function, depending on the counter
    def image_processing0(a):
        if count == 0 or count == 3 or count == 6:
            c = int(a * 0.1)
            return c

        elif count == 1 or count == 4 or count == 7:
            c = int(a * 0.5)
            return c

        elif count == 2 or count == 5 or count == 8:
            c = int(a * 0.9)
            return c
#---------------------------------------------------

#Check the count, use logic to apply the processing to the correct channel               
if count <3:
    r = image_processing0(r)
    pic.putpixel((x,y),(r,g,b))

elif count < 6:
    g = image_processing0(g)
    pic.putpixel((x,y),(r,g,b))

elif count < 9:
    b = image_processing0(b)
    pic.putpixel((x,y),(r,g,b))
#---------------------------------------------------

I tried to change my logic to the following, but soon realized that when I put the pixel back, i had no way to specify exactly what channel I was placing back in the pic.putpixel((x,y),(r,g,b)) statement.

if count <3:
    channel = r
elif count < 6:
    channel = g
elif count < 9:
    channel = b

channel = image_processing0(channel)
pic.putpixel((x,y),(r,g,b))    #No way to define which channel is being changed
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Entirely separately from the organisation of your code: multiplying sRGB coordinates by a constant fraction may not achieve what you want. See blog.johnnovak.net/2016/09/21/… \$\endgroup\$ – user3840170 Feb 8 at 16:05
6
\$\begingroup\$

Some style advice and much simpler/shorter/faster solutions.

Style

Your comments break the structure, making the code harder to read. As PEP 8 says:

Block comments [...] are indented to the same level as that code. Each line of a block comment starts with a # and a single space

So

while count <9:
#Creat Pic at the start of each loop
    pic = Image.open(file)
#Convert the image to RGB, so we dont have to deal with the alpha channel
    pic = pic.convert('RGB')

becomes

while count <9:
    # Create pic at the start of each loop
    pic = Image.open(file)
    # Convert the image to RGB, so we dont have to deal with the alpha channel
    pic = pic.convert('RGB')

and I might remove the first comment, as it, unlike the second comment, doesn't contain any insight that's not easily seen in the code anyway.

NumPy

Taking tsh's solution further, to NumPy:

from PIL import Image
import numpy as np

file = "Final/charlie.png"

im = Image.open(file).convert('RGB')
images = []
for channel in range(3):
    for factor in 0.1, 0.5, 0.9:
        data = np.copy(im)
        ch = data[:, :, channel]
        np.multiply(ch, factor, ch, casting='unsafe')
        images.append(Image.fromarray(data))

Image.convert with matrix

Or using convert's matrix parameter:

from PIL import Image

file = "Final/charlie.png"

im = Image.open(file)
images = []
for channel in range(3):
    for factor in 0.1, 0.5, 0.9:
        matrix = [1, 0, 0, 0,
                  0, 1, 0, 0,
                  0, 0, 1, 0]
        matrix[channel * 5] = factor
        images.append(im.convert('RGB', matrix))

Speeds

Using some 575 x 575 sample image, I got roughly these times per new image:

  • Yours: 2.9 seconds
  • tsh's: 0.8 seconds
  • My NumPy one: 0.003 seconds
  • My convert one: 0.005 seconds
\$\endgroup\$
2
  • \$\begingroup\$ I like the convert method used here. \$\endgroup\$ – tsh Feb 8 at 12:11
  • \$\begingroup\$ Thank you for the feedback, I have learnt a lot from this. \$\endgroup\$ – blupacetek Feb 9 at 9:53
5
\$\begingroup\$

You can handle RGB channels as a single variable color. And your function receives a color, not a channel, returns processed color.

And, you had combined two loops into one for count in range(9). They should be for factor in [0.1, 0.5, 0.9] and for channel in range(3). count is meaningless in the loop. But factor and channel are useful. So you do not need to calculate factor and channel variable from count later.

from PIL import Image

def convert_color(factor, channel):
    def convert(color):
        return (
                *color[:channel],
                int(color[channel] * factor),
                *color[channel + 1:]
        )
    return convert

pic = Image.open('image.png').convert('RGB')
processed = []
for channel in range(3): # which channel
    for factor in [0.1, 0.5, 0.9]: # factor to multiple
        new_pic = Image.new('RGB', pic.size)
        new_pic.putdata(list(map(
            convert_color(factor, channel),
            pic.getdata()
        )))
        processed.append(new_pic)
        new_pic.save(f'image{len(processed)}.png')

Just one more thing (not related to duplicated codes): getpixel and setpixel may slow. Use getdata, putdata as above code would be a better choice.

\$\endgroup\$
1
  • \$\begingroup\$ I never thought of it this way, thank you. \$\endgroup\$ – blupacetek Feb 9 at 9:56
4
\$\begingroup\$

For the image_processing0 function:

  1. You can replace things like if a == x or a == y or a == z with a simple if a in [x, y, z]

  2. The elif statements aren't necessary, as the return statements in the previous conditions already made sure that the other statements will only be evaluated if the previous condition didn't meet.

From:

def image_processing0(a):
    if count == 0 or count == 3 or count == 6:
        c = int(a * 0.1)
        return c
    elif count == 1 or count == 4 or count == 7:
        c = int(a * 0.5)
        return c
    elif count == 2 or count == 5 or count == 8:
        c = int(a * 0.9)
        return c

To:

def image_processing0(a):
    if count in [0, 3, 6]:
        c = int(a * 0.1)
        return c
    if count in [1, 4, 7]:
        c = int(a * 0.5)
        return c
    if count in [2, 5, 8]:
        c = int(a * 0.9)
        return c

For the while loop:

  1. Instead of using a while loop and constantly incrementing a variable to determine when to end the looping, use a for loop with the built-in range() method:

  2. As the count variable will never reach the if - elif statements without satisfying one of them, it is safe to unindent the repetitive line to where the if - elif statements end.

From:

count = 0

while count < 9:
    pic = Image.open(file)
    pic = pic.convert('RGB')
    for x in range(pic.size[0]):
        for y in range(pic.size[1]):
            r, g, b = pic.getpixel((x, y))
            if count < 3:
                r = image_processing0(r)
                pic.putpixel((x, y), (r, g, b))
            elif count < 6:
                g = image_processing0(g)
                pic.putpixel((x, y), (r, g, b))
            elif count < 9:
                b = image_processing0(b)
                pic.putpixel((x, y), (r, g, b))
               
    images.append(pic)   
    count += 1  

To:

for count in range(9):
    pic = Image.open(file)
    pic = pic.convert('RGB')
    for x in range(pic.size[0]):
        for y in range(pic.size[1]):
            r, g, b = pic.getpixel((x, y))
            if count < 3:
                r = image_processing0(r)
            elif count < 6:
                g = image_processing0(g)
            elif count < 9:
                b = image_processing0(b)
            pic.putpixel((x, y), (r, g, b))
               
    images.append(pic)   

So altogether:

file = "Final/charlie.png"
images = []

def image_processing0(a):
    if count in [0, 3, 6]:
        c = int(a * 0.1)
        return c
    if count in [1, 4, 7]:
        c = int(a * 0.5)
        return c
    if count in [2, 5, 8]:
        c = int(a * 0.9)
        return c
    
for count in range(9):
    pic = Image.open(file)
    pic = pic.convert('RGB')
    for x in range(pic.size[0]):
        for y in range(pic.size[1]):
            r, g, b = pic.getpixel((x, y))
            if count < 3:
                r = image_processing0(r)
            elif count < 6:
                g = image_processing0(g)
            elif count < 9:
                b = image_processing0(b)
            pic.putpixel((x, y), (r, g, b))
               
    images.append(pic)  
\$\endgroup\$
2
  • \$\begingroup\$ Thank you kindly for the great information, and flow of logic. This certainly helps me think and lean. Have a great weekend. \$\endgroup\$ – blupacetek Feb 7 at 16:24
  • \$\begingroup\$ The elif statements can be made into ifs, but I'd prefer to leave them as elif statements, as it visually indicates to any future readers that these statements are joined and mutually exclusive. Sure that's obvious to anyone with half a brain in this particular code snippet, but for a more complex piece of code it might not be, and as far as I know there's no cost to leaving them as elif. \$\endgroup\$ – Arcanist Lupus Feb 8 at 1:55
2
\$\begingroup\$

Chocolate's advice for reducing repetition is excellent, but we can go further.

We'll start with their version of the image_processing0 function.

def image_processing0(a):
    if count in [0, 3, 6]:
        c = int(a * 0.1)
        return c
    if count in [1, 4, 7]:
        c = int(a * 0.5)
        return c
    if count in [2, 5, 8]:
        c = int(a * 0.9)
        return c

First, the if checks can be simplified

def image_processing0(a):
    if count%3 == 0: # 0, 3, 6
        c = int(a * 0.1)
        return c
    if count%3 == 1: # 1, 4, 7
        c = int(a * 0.5)
        return c
    if count%3 == 2: # 2, 5, 8
        c = int(a * 0.9)
        return c

Now, notice that the only difference between these if blocks is the value of the modifier.

def image_processing0(a):
    if count%3 == 0: # 0, 3, 6
        mod = 0.1
    if count%3 == 1: # 1, 4, 7
        mod = 0.5
    if count%3 == 2: # 2, 5, 8
        mod = 0.9
    c = int(a * mod)
    return c

This in turn opens us up to a clever little shortcut where you can use a dict as a simple case statement

MODIFIERS = {0:0.1, 1:0.5, 2:0.9}
def image_processing0(a):
    mod = MODIFIERS[count%3]
    c = int(a * mod)
    return c

In this particular case, the keys to the dict are the same as the indexes of a list, so we can use a list instead. Also, I've taken the liberty of adding count as an argument in preference to referencing it as a global variable, and condensing the return statement into the previous line.

#Image processing function, depending on the counter
MODIFIERS = [0.1, 0.5, 0.9]
def image_processing0(a, count):
    mod = MODIFIERS[count%3]
    return int(a * mod)

We can use a similar trick to simplify which color is being modified

#Don't split the colors, but keep them in a single list
colors = list(pic.getpixel((x,y)))
            
#Get the index needed from the count
idx = count//3

#Set that color to its modified value
colors[idx] = image_processing0(colors[idx], count)

#Set the pixel to its modified color
pic.putpixel((x,y), colors)

Bring it all together, and you have your new script!

#%%

from PIL import Image

# Set the file and open it
file = "Final/charlie.png"

#Create an empty list to append the images to
images = []

MODIFIERS = [0.1, 0.5, 0.9]
COLOR_MODEL = 'RGB'

#Image processing function, depending on the counter
def image_processing0(a, count):
    mod = MODIFIERS[count%len(MODIFIERS)]
    return int(a * mod)

#Create PixelMap
for count in range(len(COLOR_MODEL)*len(MODIFIERS)):
#Creat Pic at the start of each loop
    pic = Image.open(file)
#Convert the image to RGB, so we dont have to deal with the alpha channel
    pic = pic.convert(COLOR_MODEL)
    
#Get the RGB value for every pixel
    for x in range(pic.size[0]):
        for y in range(pic.size[1]):

            #Get the pixel colors
            colors = list(pic.getpixel((x,y)))
            
            #Set new colors
            idx = count//len(MODIFIERS)
            colors[idx] = image_processing0(colors[idx], count)
            pic.putpixel((x,y),colors)

#Add the images to the list                    
    images.append(pic)

No repetitive if statements at all!

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Kinda odd to split the modifiers information to inside/outside the function (outside the values, inside how to pick one (especially that there are three)). I'd just do mod = (0.1, 0.5, 0.9)[count % 3] inside the function. \$\endgroup\$ – Kelly Bundy Feb 8 at 11:01
  • \$\begingroup\$ @KellyBundy There's a minor computational gain by not having to recreate the list every time the function is called, but if you care about speed and efficiency you should really be using a numpy solution like yours anyways. More importantly, I like to pull the 'magic numbers' out of my functions and define them as constants before hand, where they're easy to adjust. \$\endgroup\$ – Arcanist Lupus Feb 9 at 3:07
  • \$\begingroup\$ Thank you kindly, its been a huge learning journey for me to see all of these alternative approaches. I will experiment with them in order to gain a little more skill. \$\endgroup\$ – blupacetek Feb 9 at 9:55
  • \$\begingroup\$ "not having to recreate the list every time" - That's why I made it a tuple. So that it's just a LOAD_CONST. And you still have the magic number 3 inside the function, separate from the magic data in your list outside the function even though they rather belong together. \$\endgroup\$ – Kelly Bundy Feb 9 at 13:37
  • \$\begingroup\$ Did a benchmark now, my constant-tuple way actually looks slightly faster than yours (see the three last lines on the bottom of the output). \$\endgroup\$ – Kelly Bundy Feb 9 at 13:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.