Don't report -1 as a factor

[jhenahan]

Fixes #95

Not sure if abs g == abs g' is the best way to go about this in the property. Feels a little too forgetful, but I'm not sure how else to reformulate the test to get it to pass.

[Bodigrim]

When the first argument is an empty list, abs g' is guaranteed to be 1, so we can just return [].
In fact, now helper does not need to carry the second argument at all.

[jhenahan]

I've reduced the base case to

helper [] _ = []

but I'm less certain about how to rework the other case to remove the need for g', assuming that's what you mean.

[Bodigrim]

My fault, it is non-trivial to remove the need for g'. Definitely out-of-scope for this PR.

[Bodigrim]

Please keep code aligned by =.

[Bodigrim]

I guess we can safely remove check g == g', because it is a specific instance of abs g == abs g'.

[Bodigrim]

Thanks for your contribution! Please find review suggestions above.

[jhenahan]

Updated, except as commented above.

EDIT: Review commit squashed now that it seems we're all good. :)

[Bodigrim]

Please amend the haddock comment to reflect your changes.

[Bodigrim]

Please amend the haddock comment to reflect your changes.

[Bodigrim]

It is worth to clarify in haddock comment that unit factors are not included.

[jhenahan]

Haddock comments updated.

[Bodigrim]

Excellent!

There is one remaining area to improve: Math.NumberTheory.UniqueFactorisation. Since negative numbers do not cause -1 as a factor anymore, instances of UniqueFactorisation can be simplified. E. g.,

 instance UniqueFactorisation Int where
   unPrime   = coerce wordToInt
-  factorise m' = if m <= 1
-                then []
-                else map (coerce integerToWord *** intToWord) . F.factorise' . intToInteger $ m
-                  where
-                    m = abs m'
+  factorise = map (coerce integerToWord *** intToWord) . F.factorise . intToInteger

Make sure to rebase your branch on latest master commit (317c841), please.

[jhenahan]

I'll chase down the test failures later on.

[Bodigrim]

Just replace F.factorise' by F.factorise to fix most of the failures.

[Bodigrim]

I looked into two remaining failures. There are several ways to fix them, but the most fundamental one is to change default implementation of Math.NumberTheory.UniqueFactorisation.isPrime to handle a special case isPrime 0 = Nothing before resorting to factorise. This will require enabling {-# LANGUAGE DefaultSignatures #-}.

[jhenahan]

@Bodigrim Such a definition also implies a (Num a, Eq a) constraint on the UFD class, which gets us into trouble in a few other places. It's sufficient to add the special case for Int to get the tests to pass, but I'm not sure if you'd prefer a more thorough reformulation to make the special case unnecessary.

[Bodigrim]

Such a definition also implies a (Num a, Eq a) constraint on the UFD class, which gets us into trouble in a few other places.

That is why I suggested enabling {-# LANGUAGE DefaultSignatures #-}. One can define isPrime this way:

  isPrime   :: a -> Maybe (Prime a)

  default isPrime :: (Eq a, Num a) => a -> Maybe (Prime a)
  isPrime 0 = Nothing
  isPrime n = case factorise n of
    [(p, 1)] -> Just p
    _        -> Nothing

Then for types, satisfying (Eq a, Num a), isPrime will be derived automatically.

Another option is proceed further with your approach, removing default implementation of isPrime entirely and defining it by hand in each of five instances. Less fancy, more robust, so it is a matter of taste.

[jhenahan]

I've gone with the DefaultSignatures approach for now, but I'd really like to work out a more robust solution to this issue. It might require building out a more expressive typeclass hierarchy to get up to UFDs, though, so it's not at all a blocker for this merge if things otherwise look good.

[Bodigrim]

Yeah, it is quite possible that another hierarchy will work better, especially when we get quadratic numbers (#82) merged. UniqueFactorisation is still a liquid metal.

Merged, many thanks for your awesome work.