3

I'm currently working on a program which computes amicable pairs (Project Euler Problem 21). I've already found the solution, however I noticed that a flaw in my program was that it evaluates all of the numbers of the set [1..] whether or not we have already found the number to be a pair.

i.e. If currently evaluating 220 and 284 is found to be it's pair, however continuing on through when the map function gets to 284 it shouldn't evaluate it again.

import Data.List

properDivisors :: (Integral a) => a -> [a]
properDivisors n = [x | x <- [1..n `div` 2],
                        n `mod` x == 0 ]

amicablePairOf :: (Integral a) => a -> Maybe a
amicablePairOf a
    | a == b = Nothing
    | a == dOf b = Just b
    | otherwise = Nothing
        where dOf x = sum (properDivisors x)
              b = dOf a

getAmicablePair :: (Integral a) => a -> [a]
getAmicablePair a = case amicablePairOf a of
            Just b -> [a,b]
            Nothing -> []


amicables = foldr (++) [] ams
    where ams = map getAmicablePair [1..]

As an example:

take 4 amicables

returns:

[220,284,284,220]

I'm fairly new to Haskell and functional programming so forgive me if it an obvious solution.

Zero Piraeus
  • 56,143
  • 27
  • 150
  • 160
Emmanuel M. Smith
  • 431
  • 1
  • 5
  • 12
  • is this amicable-relation symmetric? If yes, Fuzzxxl's solution is fine. If not ... then I would doubt that its sensible to avoid those calculations. – phynfo May 31 '11 at 12:00
  • 1
    @phynfo: amicable means that two numbers have the special relationship that the sum of their divisors equals the other number. So it is indeed symmetric. There are only 1427 amicable numbers below 10000000000, so it is not worth the effort to avoid calculations for such a small fraction. A good optimization would be to check, if the number is deficient, whether the sum is already included in the list. That would safe some time, as one could avoid the second sum in about 50% of the numbers. – fuz May 31 '11 at 12:21

2 Answers2

5

Your problem is, that you try to safe work by outputting both amicable numbers. But actually, you don't safe very much, because your function still calculates for both numbers, whether they are amicable. Why not do it like this:

import Data.List

divSum :: (Integral a) => a -> [a]
divSum n = sum (filter (\a -> a `mod` n == 0) [1..n `div` 2])

isAmicable :: (Integral a) => a -> Bool
isAmicable a = a /= b && a == c where
  b = divSum a
  c = divSum b

amicables = filter isAmicable [1..]
fuz
  • 88,405
  • 25
  • 200
  • 352
  • There is the famous quote from Don Knuth: *»Premature optmization is the root of all evil.«* Usually, the best way is to code in a straightforward way and profile afterwards. If the results are not satisfying, try to opitmize the code. – fuz May 31 '11 at 12:05
  • the best way is to code in a straightforward way and profile afterwards *if you need to*. If you got your answer, who cares how long it took in the past? :-) – luqui May 31 '11 at 22:57
2

Perhaps a slight modification in getAmicablePair helps?

getAmicablePair :: (Integral a) => a -> [a]
getAmicablePair a = case amicablePairOf a of
            Just b -> if a < b then [a,b] else []
            Nothing -> []

... so you just get pairs with a smaller first element

phynfo
  • 4,830
  • 1
  • 25
  • 38
  • Good solution, however the problem is that the program would still evaluate the pair twice, it just will not show it the second time. I know how to remove duplicates using the 'nub' function from Data.List, however I want to improve efficiency by only evaluating a pair once. – Emmanuel M. Smith May 31 '11 at 11:40
  • @Axilus - youre right ... but then you can't use `map`, since `map` cannot refer to earlier calcs. – phynfo May 31 '11 at 11:44
  • @Axilus: I doubt that you would improve the efficiency by calculating them only once. First, you have to do a lookup in the list of all amicable numbers already generated, so your function doesn't runs in constant space anymore (assuming, that you consume the results instantly). Additionally, only a tiny fraction of all numbers are amicable, so you don't win anything either. – fuz May 31 '11 at 11:47