3
\$\begingroup\$

This is what I have so far but it's not perfect... Any input would be very helpful!

def picture_reset_pixels(filename, from_color, to_color, target_color):
    img = Image.open(filename)
    pixels = list(img.getdata()) 
    red_list=[]
    for i in pixels: 
        R= i[0]
        G=i[1] 
        B=i[2] 

        if R >= from_color[0] and R<= to_color[0] and G >=from_color[1] 
                and G<= to_color[1] and B >= from_color[2] and B<= 
                to_color[2]:
                    red_list.append(target_color)
        else:
            red_list.append((round(R),round(G),round(B))) 

    red_image = Image.new(img.mode,img.size)
    red_image.putdata(red_list) 
    return red_image
\$\endgroup\$

closed as off-topic by Graipher, Toby Speight, Mast, Malachi Nov 24 '18 at 16:04

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Toby Speight, Mast, Malachi
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    \$\begingroup\$ Your if statement is split over three lines, in a way that I think is not valid in Python. If you put brackets around it, then you can break it up across lines: stackoverflow.com/questions/5253348/… \$\endgroup\$ – Cris Luengo Nov 16 '18 at 23:00
  • 2
    \$\begingroup\$ Also clarify what imports apply. What are Image and round? \$\endgroup\$ – 200_success Nov 17 '18 at 1:27
  • 1
    \$\begingroup\$ @200_success round is a built-in function. For the Image my guess is it comes from pillow. \$\endgroup\$ – Arthur Havlicek Nov 17 '18 at 2:12
  • \$\begingroup\$ This question is lacking context. What are the used imports and what is the code supposed to do? \$\endgroup\$ – Mast Nov 23 '18 at 11:00
2
\$\begingroup\$

Improving the general implementation

Generally we want to avoid appending to a list too many times if we can help it. This is because lists are dynamic arrays, so appends can be unnecessarily expensive (though the dynamic part helps with the efficiency). Instead, when we know the size of the input set (i.e. the number of pixels in the image), we can take advantage of that information. But in this case, that will be unnecessary because Image provides us with some handy helper methods to handle this very situation:

from PIL import Image

def picture_reset_pixels(filename, from_color, to_color, target_color):
    img = Image.open(filename)
    pixels = img.load()
    for i in range(img.size[0]):
        for j in range(img.size[1]):
            if all(from_color[k] <= pixels[i, j][k] <= to_color[k] for k in range(3)):
                pixels[i, j] = target_color + (255,)
    return img

Assigning pixels to img.load() allows us to take advantage of directly editing the image. Since copying the image into another list was unnecessary originally (to clarify, the img variable doesn't affect the file on disk that img originated from, i.e. the file at location filename), editing directly is the best way.

Edit 2: Looking at it again, I think @Reinderien's point about clarity has merit because range has an exclusive upper bound, which may be confusing because it is asymmetric relative to both of its arguments. I have edited to use the comparison operators <= and >= instead of the range check I mention ahead. The caveat only I have is that using the in range form is more similar to how I think a dedicated ColorRange object (mentioned ahead in the section Going beyond the current form) should handle a range check, since it's not necessary to expose the object's implementation details (though internally, it could still use comparison operators for the check).

Another general improvement I've taken advantage of in this implementation is using range to check if each of the RGB values of a pixel are between the two pixel boundaries of the input arguments. all() serves the purpose that the and statements in the original version served: it requires that all the pixels compared in the iterator pixels[i, j][k] in range(from_color[k], to_color[k] + 1) for k in range(3) are in the required range for the condition to be true.

Edit 1: the concern was raised in comments that testing for in range(from_color[k], to_color[k] + 1) would be extremely inefficient. However, this is mistaken and arises from a misunderstanding of how Python 3.x implement the range object. Checking if a number is in a range is a constant time operation. I would recommend reading this answer to understand Python 3.x's range implementation.

Going beyond the current form

While I've shown you a way to use your current parameter requirements in a more efficient implementation, I think there are some improvements you can make so picture_reset_pixels has more functionality. I think combining from_color, to_color, and target_color into a single dict parameter color_replacements could make this more versatile (where the key-value pairs would derive something like {(from_color, to_color): target_color}; you could even create a custom ColorRange class to encapsulate the idea of a range of colors).

There's also another issue I find with your current implementation: it can only support certain image types. I tried your algorithm on a GIF file and it failed because the pixels in a GIF file are ints, not four-tuples like PNGs. You also do not support the fourth alpha channel of PNG, because your pixel range bounds are only 3-tuples. You might want to consider investigating that further if you want to support multiple image types.

\$\endgroup\$
  • \$\begingroup\$ @Reinderien That would be true if this were Python 2.x. But in Python 3.x, range is the equivalent of Python 2.x's xrange and returns an iterator. Python 3.x also added an optimization for the .__contains__() method so that it is O(1) time. \$\endgroup\$ – Graham Nov 17 '18 at 12:37
  • \$\begingroup\$ It smells, to me. Using a pair of inequalities is simpler, and no less legible. \$\endgroup\$ – Reinderien Nov 18 '18 at 4:10
  • \$\begingroup\$ @Reinderien Looking at it again, I think I agree with you. The exclusive upper bound of range can be a bit confusing. And Python does have this neat interval comparison where A <= X and X <= B can be written as A <= X <= B so it's not redundant and synonymous to its math form. The caveat only I have is that using the in range form is more similar to how I think a dedicated ColorRange object should handle a range check, since it's not necessary to expose the object's implementation details (though internally, it could still use comparison operators for the check). \$\endgroup\$ – Graham Nov 18 '18 at 12:27
2
\$\begingroup\$

Currently you are manipulating the image one pixel at once. Instead you can work on the whole array as one when using the numpy interface:

from PIL import Image
import numpy as np

def read_image(image_path):
    img = Image.open(image_path)
    return np.asarray(img, dtype="int32")

def write_image(img, image_path):
    img = Image.fromarray(np.asarray(img, dtype="uint8"), "RGBA")
    img.save(image_path)

def picture_reset_pixels(img, from_color, to_color, target_color):
    mask = (img >= from_color).all(axis=-1) & (img <= to_color).all(axis=-1)
    img[mask] = target_color

if __name__ == "__main__":
    from_color = (100, 100, 100, 255)
    to_color = (150, 150, 150, 255)
    target_color = (0, 0, 0, 255)
    filename = "test.png"
    img = read_image(filename)
    picture_reset_pixels(img, from_color, to_color, target_color)
    write_image(img, filename)

Note that I chose not to return the modified image since the function modifies it inplace. If you want to avoid that, just add img = img.copy() as the first line and add return img at the end.

Also note that this assumes that your image has an alpha channel. If not you need to change "RGBA" to "RGB" and of course have the colours be only three-tuples.

\$\endgroup\$
1
\$\begingroup\$

The code is overall simple and good but could be improved in a few things that would make it a bit shorter and more readable


Inside the for loop, in the if / else, you are repeating append in both of your cases. You could do instead

to_add = (round(R), round(G), round(B))
if condition:
    to_add = target_color
red_list.append(to_add)

On the condition, you can also refactor using chained comparison to improve readability, preferably in ascending order. If you break that line, do so aligning conditionals as follow :

if from_color[0] <= R <= to_color[0] and \
   from_color[1] <= G <= to_color[1] and \
   from_color[2] <= B <= to_color[2]:

Alternatively, but sightly less explicit, you can iterate over i.

if all((from_color[j] <= i[j] <= to_color[j] for j in range(3)))

R, G, B initialisation can be made a single line:

R, G, B = i

or, if working with a 4 tuples:

R, G, B, _ = i
\$\endgroup\$

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