aleator / CV

Haskell wrappers and utilities for OpenCV machine vision library
http://hackage.haskell.org/package/CV
BSD 3-Clause "New" or "Revised" License
51 stars 13 forks source link

Matrix (and possibly others) breaks referential transparency #22

Closed jochu closed 12 years ago

jochu commented 12 years ago

Problem

I've noticed that here and there that matrices are being created in pure haskell (via unsafePerformIO) and then being modified in an FFI IO call. Not only does this break referential transparency, but it can lead to hard to debug errors after compiler optimization.

Example

I ran across this when I was writing an FFI entry for cvGetPerspectiveTransform. I followed some existing patterns from elsewhere for dealing with matrices (see HoughTransforms and Calibration), and did something like:

let m = create (3, 3) :: Matrix Float
-- ...
withMatPtr m $ \mat ->
     c'cvGetPerspectiveTransform c_from c_to mat
-- ...
return $ M.toList m

This yielded incorrect responses - giving the default values from when m was created and not the newly modified values from after the perspective transform.

Temporary Fix

To temporarily remedy this problem, I created IO versions of Matrix functions without the usage of unsafePerformIO. Specifically, createM, toColsM, and toRowsM.

Scope

This pattern of breaking referential transparency also seems to exist elsewhere. One example for the Image type can be found in the definition of convolve2D. It may exist in several other places as well, but I have not checked. While I haven't seen it break for other uses yet, it does make me fairly nervous.

Fixing this to prevent referential transparency errors will likely lead to breaking changes in the API.

aleator commented 12 years ago

This is quite embarrassing. I've now changed moved create to IO where it should be. This should fix the issue without need to for toColsM and toRowsM. I also attempted to find all the places where this pattern exists and to remove it.

I'm closing the issue for now, but please comment if this doesn't work for you.