lehins / massiv

Efficient Haskell Arrays featuring Parallel computation
BSD 3-Clause "New" or "Revised" License
384 stars 23 forks source link

odd behaviour of Sz1 when subtracting something #114

Closed ilcasinistareloaded closed 3 years ago

ilcasinistareloaded commented 3 years ago

Hello again. There is something odd going on here. With Int it works OK but with Sz1 there is a problem. (GHC 8.10.4; massiv 0.6). Thanks!

import Data.Massiv.Array (Sz(..), Sz1)

f  :: Sz1 -> Sz1
f   = ($) (\ x -> x - 1)

f' :: Sz1 -> Sz1
f' = ($) (+ (- 1))

ok  = f  (Sz 10) -- 9
bug = f' (Sz 10) -- 10 <-- THIS SHOULD BE 9.
lehins commented 3 years ago

Hello again, and you again find some more odd behavior with regards to Num ;)

This is a very good observation. However, it is a totally explainable one: Sz cannot be negative by construction. So when you use prefix -, eg: -1 :: Sz1 it will be desugared into Sz (max 0 (fromInteger (negate 1))) == Sz 0, which is different from the infix -, eg. 5 - 1, because latter is desugared into Sz (max 0 (fromInteger 5) - max 0 (fromInteger 1))

This is not the first place in Haskell when this behavior related to how numeric literals are desugared produces surprising results. In fact you can replicate similar behavior with the Natural:

import Numeric.Natural

f  :: Natural -> Natural
f   = ($) (\ x -> x - 1)

f' :: Natural -> Natural
f' = ($) (+ (- 1))

ok  = f  10 -- 9
bug = f' 10 -- 10 <-- THIS SHOULD BE 9.

Which results in:

λ> ok
9
λ> bug
*** Exception: arithmetic underflow

We could fix this deficiency for Sz by producing an error instead of silently enforcing non-negative sizes with max, but I am leaning towards removing Num instance for Sz because this sort of behavior is unexpected and undesirable no matter how you look at it. Maybe, we should do both.

The proper workaround is to unwrap Sz and operate on Ix when doing arithmetic and then construct Sz back when size is needed.

Thank you for pointing out this deficiency.

ilcasinistareloaded commented 3 years ago

Clear explanation, thanks! I didn't know there was a similar issue with Natural. If that is a common knowledge I think this issue can be closed with just a comment in the docs about this pitfall.