Image processing using Python OOP library

Question

I am trying to learn OOP by refactoring my existing code (functions only). This example works but I am not sure whether it is pythonic and good practice.

I have a shared library which consist of several file with functions:

color_spaces.py

import cv2

def rgb_to_hsv(image):
    print ('hsv')
    hsv_image = cv2.cvtColor(image, cv2.COLOR_RGB2HSV)
    hue, sat, val = hsv_image[:, :, 0], hsv_image[:, :, 1], hsv_image[:, :, 2]
    return hsv_image, hue, sat, val

def rgb_to_lab(image):
    lab_image = cv2.cvtColor(image, cv2.COLOR_RGB2LAB)
    l_chan, a_chan, b_chan = lab_image[:, :, 0], lab_image[:, :, 1], lab_image[:, :, 2]
    return lab_image, l_chan, a_chan, b_chan

thresholding.py

import cv2


def treshold_otsu(gray_image):
    ret, thresholded_image = cv2.threshold(gray_image, 0, 255, cv2.THRESH_BINARY_INV + cv2.THRESH_OTSU)
    return thresholded_image

def global_threshold(gray_image):
    ret, thresholded_image = cv2.threshold(gray_image, 127, 255, cv2.THRESH_BINARY)
    return thresholded_image

My class which uses functions of the shared library:

pipline01.py

import cv2
from color_spaces import rgb_to_hsv, rgb_to_lab
from thresholding import treshold_otsu, global_threshold

class Segmentation1(object):
    def __init__(self, filename, path, procedure):
        self.filename = filename
        self.path = path
        self.procedure = procedure

    def start_pipline(self):
        # Read image
        image = cv2.imread(self.path + self.filename)

        # Do segmentation procedure depending on procedure
        if self.procedure == 'test01':
            source = rgb_to_hsv(image)[1]
            binary = treshold_otsu(source)
        elif self.procedure == 'test02':
            source = rgb_to_lab(image)[1]
            binary = global_threshold(source)

        # Save image
        cv2.imwrite(self.path + self.filename.split('.')[0] + '_' + self.procedure + '.png', binary)

Then I call the class in a batch mode by looping over a folder with images:

batch_pipline01.py

import os
import argparse
from pipline01 import Segmentation1

parser = argparse.ArgumentParser(description = 'Test.')
parser.add_argument('-d', '--img_dir', required = True,
                    help = 'directory containing images to segment.')
parser.add_argument('-p', '--procedure', required = True,
                    help = 'procedure to run.')

args = parser.parse_args()
print(args)
img_dir = args.img_dir
procedure = args.procedure
images = os.listdir(img_dir)
#images = [img[2:] for img in args.images]

# Start segmentation for each image
for img in images:
    os.chdir(img_dir)
    print('SEGMENTING ' + img)
    segmenter = Segmentation1(img, img_dir, procedure)
    obj = segmenter.start_pipline()
    del obj

Can I improve my code and design?

Answer 1

This scenario is not best case for doing an OOP conversion: In this example you're not getting real assistance from the Segmentation1 class over what you'd get by using static functions. The code looks like it does what you need done; it's simply not a great application for OOP as written. At some point you should check out this talk by Python core dev Jack Diederich for some perspective on when OOP helps in Python and when it doesn't. One of the main takeways there is that classes which expose just one method ususally don't need to be classes.

That said:

If you want to use this as learning case, you can refactor your arrangement so that you have two different classes for the two different strategies. This is overkill in the code as is, but if you anticipated a much wider range of segmentation tasks with different inner workings it might make sense. It would be a good choice if the segmentation operation had multiple steps and needed to maintain state within a single run -- for example, if it had to collect data from multiple data sources and coordinate it differently for LAB colors and HSV colors.

Your existing code really has only two paths, splitting on the procedure. So that's a good clue that you want the sibling classes to differ primarily according to the differences -- the rule is to "encapsulate what varies" -- and, conversely, to share the parts that are the same. The file handling and so on are the same. So first you can isolate the unchanging bits into mini-methods of their own, and then bookend them around the parts that will change:

class SegmentationType(object):
    DISPLAY_NAME = "invalid"

    def __init__(self, filename, path):
        self.filename = filename
        self.path = path
        self.input_data = None
        self.output_data = None


    def read_image(self):
        self.input_data =  cv2.imread(self.path + self.filename)[1]

    def write_image(self):
        cv2.imwrite(self.path + self.filename.split('.')[0] + '_' + self.DISPLAY_NAME + '.png', self.output_data)

    def process(self):
        # override in derived classes to perform an actual segmentation
        pass

    def start_pipeline(self):
        self.read_image()
        self.process()
        self.write_image()

Then you just need to create subclasses for your strategies:

class HSV_Segmenter(SegmentationType):
    DISPLAY_NAME = 'HSV'

    def process(self):
        source = rgb_to_hsv(self.input_data)
        self.output_data = treshold_otsu(source)


class LabSegmenter(SegmentationType):
    DISPLAY_NAME = 'LAB'

    def process(self):
        source = rgb_to_lab(self.input_data)
        self.output_data = global_threshold(source)

This should be equivalent to your earlier code -- it does have one minor advantage, which is that it will fail at compile time if you don't pick the proper subclass where the original would fail at runtime if you accidentally pass a bad value for procedure. Then your program simply chooses the right class and calls it:

segmenter_class = {
    'hsv': HSV_Segmentation,
    'lab': LAB_Segmenter
}.get(procedure)

if not segmenter_class:
    raise ArgumentError("Invalid segmentation method '{}'".format(procedure))

for img in images:
    os.chdir(img_dir)
    processor =  = segmenter_class(img, img_dir, procedure)
    processor.start_pipeline()

Again, I personally would stick with static functions in this situation. In Python you could achieve all the good parts of the class arrangement by just having that dictionary lookup return the right static functions instead of classes. Lots of languages don't have that neat function and so this kind of structure is very common there.