Revisions to PPM/PGM Image processing library in Haskell

added 184 characters in body

Source Link

edited Jan 29, 2014 at 2:38

331
1
5

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

While this looks like it's cheap, beware that those Ints are boxed values. This means that they can hold either thunks, or pointers to Ints. This extra layer of indirection is potentially harmful to performance, so try unpacking the values directly into the record:

{-# LANGUAGE BangPatterns #-}
data ColorPixel = ColorPixel {-# UNPACK #-} !Int {-# UNPACK #-} !Int {-# UNPACK #-} !Int
    deriving Show

note: compiling with -funbox-strict-fields allows you to omit the UNPACK pragmas and have the compiler add them for you.

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

EDIT: Incorporated cdk's feedback re: UNPACK

REF: http://www.haskell.org/ghc/docs/7.0.2/html/users_guide/pragmas.html#unpack-pragma

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

While this looks like it's cheap, beware that those Ints are boxed values. This means that they can hold either thunks, or pointers to Ints. This extra layer of indirection is potentially harmful to performance, so try unpacking the values directly into the record:

data ColorPixel = ColorPixel {-# UNPACK #-} Int {-# UNPACK #-} Int {-# UNPACK #-} Int
    deriving Show

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

EDIT: Incorporated cdk's feedback re: UNPACK

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

While this looks like it's cheap, beware that those Ints are boxed values. This means that they can hold either thunks, or pointers to Ints. This extra layer of indirection is potentially harmful to performance, so try unpacking the values directly into the record:

{-# LANGUAGE BangPatterns #-}
data ColorPixel = ColorPixel {-# UNPACK #-} !Int {-# UNPACK #-} !Int {-# UNPACK #-} !Int
    deriving Show

note: compiling with -funbox-strict-fields allows you to omit the UNPACK pragmas and have the compiler add them for you.

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

EDIT: Incorporated cdk's feedback re: UNPACK

REF: http://www.haskell.org/ghc/docs/7.0.2/html/users_guide/pragmas.html#unpack-pragma

added 184 characters in body

Source Link

edited Jan 29, 2014 at 2:31

Thomas

331
1
5

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

whileWhile this looks like it's cheap, beware that those Ints are boxed values. This means that they can potentially hold either thunks, or pointers to intsInts. It would be very rare that you would ever need just one color componentThis extra layer of indirection is potentially harmful to performance, so try using strictness annotations hereunpacking the values directly into the record:

data ColorPixel = ColorPixel {-# LANGUAGE BangPatternsUNPACK #-}
data ColorPixelInt ={-# ColorPixelUNPACK !Int#-} !Int !{-# UNPACK #-} Int
    deriving Show

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

EDIT: Incorporated cdk's feedback re: UNPACK

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

while this looks like it's cheap, beware that those Ints are boxed values. This means that they can potentially hold thunks, or pointers to ints. It would be very rare that you would ever need just one color component, try using strictness annotations here:

{-# LANGUAGE BangPatterns #-}
data ColorPixel = ColorPixel !Int !Int !Int deriving Show

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

While this looks like it's cheap, beware that those Ints are boxed values. This means that they can hold either thunks, or pointers to Ints. This extra layer of indirection is potentially harmful to performance, so try unpacking the values directly into the record:

data ColorPixel = ColorPixel {-# UNPACK #-} Int {-# UNPACK #-} Int {-# UNPACK #-} Int
    deriving Show

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

EDIT: Incorporated cdk's feedback re: UNPACK

Source Link

created Jan 28, 2014 at 23:25

Thomas

331
1
5

Regarding the problem selecting the right image type, you have enough information in your header to select which type of image to create.

  case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (header,_) -> case (imageEncoding header) of
      ASCII -> putStrLn $ source ++ ": Already an ASCII image"
      Binary -> let image = decode content :: Maybe PPMImage
                in convertImage image dest

Becomes

  image <- case header of
    Nothing -> putStrLn $ source ++ ": Bad image"
    Just (Header _ ASCII _ _) ->
      putStrLn $ source ++ ": Already an ASCII image"
    Just (Header PPM _ _ _) -> decode content :: Maybe PPMImage
    Just (Header PGM _ _ _) -> decode content :: Maybe PGMImage
  convertImage image dest

Besides, this is a check that should really be happening inside your call to decode. Ask yourself what would happen if your user called decode content :: Maybe PGMImage with PPM encoded content.

Perhaps consider leaving the image format at the value level?

newtype Image pix = MkImg (Array Coord pix) deriving Show

decode PPM content :: Image p

Following up on cdk's answer: if you are worried about speed, you should take care how you represent your data.

data ColorPixel = ColorPixel Int Int Int deriving Show

while this looks like it's cheap, beware that those Ints are boxed values. This means that they can potentially hold thunks, or pointers to ints. It would be very rare that you would ever need just one color component, try using strictness annotations here:

{-# LANGUAGE BangPatterns #-}
data ColorPixel = ColorPixel !Int !Int !Int deriving Show

You should also do this for all performance sensitive single field datatypes that you don't convert into newtypes.

Stack Exchange Network

Return to Answer