Speed up factorisation of Gaussian primes #116
Conversation
| asbs = map (\b' -> ((b' * c) `mod` p, b')) bs | ||
| (a, b) = head [ (a', b') | (a', b') <- asbs, a' <= k] | ||
| in a :+ b | ||
| findPrime' p = case Moduli.sqrtModMaybe (-1) (FieldCharacteristic (PrimeNat . integerToNatural $ p) 1) of |
There was a problem hiding this comment.
Not related to this PR, but it seems odd to have findPrime' when there is no findPrime. Is it worth renaming to findPrime, and keeping findPrime' around as a deprecated name?
|
|
||
| -- |Compute the prime factorisation of a Gaussian integer. This is unique up to units (+/- 1, +/- i). | ||
| -- Unit factors are not included in the result. | ||
| factorise :: GaussianInteger -> [(GaussianInteger, Int)] | ||
| factorise g = helper (Factorisation.factorise $ norm g) g | ||
| factorise g = concat $ snd $ mapAccumL go g (Factorisation.factorise $ norm g) |
There was a problem hiding this comment.
Is there a source for this algorithm (that might be worth citing from the doc comment)?
There was a problem hiding this comment.
The idea is to avoid repetitive complex divisions with the same divisor, because they unnecessary recalculate its norm. I'll refactor code to make it more palatable.
There was a problem hiding this comment.
Ah, I see. I think I was thrown off by the extra units that entered into the equations (to make the math work out nicer).
| gcdGProperty2 :: GaussianInteger -> GaussianInteger -> GaussianInteger -> Bool | ||
| gcdGProperty2 z z1 z2 | ||
| = z == 0 | ||
| || gcdG z1' z2' `remG` z == 0 |
There was a problem hiding this comment.
Is this a bug? How can the GCD be zero? (Or am I reading this wrong?)
There was a problem hiding this comment.
I don't think it's asking of the gcd is zero, but if the remainder of the division of that gcd by z is 0.
There was a problem hiding this comment.
It is convenient to set gcd 0 0 equal to 0.
There was a problem hiding this comment.
I see, I was parsing the code incorrectly.
|
LGTM, thanks! |
|
@cfredric Thanks for review! |
This PR resolves #111.
@cfredric would you be interested to review?