Extend 'smoothOver' and 'smoothOverInRangeBF' to 'Euclidean' types #138
Conversation
|
Cool! Could you please add tests for Gaussian and Eisenstein integers? |
|
@Bodigrim the test I just added was all I could think of that wasn't redundant. Since |
|
@Bodigrim by the way, this is good to go on my end. |
|
Sorry for delay, I'll take a look tomorrow. |
Math/NumberTheory/SmoothNumbers.hs
Outdated
| -- | Helper used by @smoothOver@. Since the typeclass constraint is just | ||
| -- @Euclidean@, with Euclidean domains not being required to have a total | ||
| -- ordering defined, it receives a @norm@ comparison function for the | ||
| -- generated smooth numbers. |
There was a problem hiding this comment.
It is worth to clarify in a comment that smoothOver' crucially relies on the fact that for any element of smooth basis p and any a we have norm (a * p) > norm a.
Otherwise iterate (map (p*)) l) may produce unsorted lists and it will be wrong to merge them together using mergeListLists.
| -- >>> take 10 (smoothOver (fromJust (fromList [2, 5]))) | ||
| -- [1, 2, 4, 5, 8, 10, 16, 20, 25, 32] | ||
| smoothOver :: (E.Euclidean a, Ord a) => SmoothBasis a -> [a] | ||
| smoothOver = smoothOver' abs |
There was a problem hiding this comment.
The problem with abs is that it does not satisfy a prerequisite of smoothOver' stated in the comment above. For instance, abs (2 + ι) > abs ((2 + ι) * (1 + ι)). It means that mergeListLists is fed with unsorted (in terms of abs) lists and consequently returns unsorted (again in terms of abs) results.
For example, notice an unexpected position of 3+4*ι:
> take 10 $ map abs $ smoothOver $ fromJust $ fromList [1+ι, 2+ι]
[1,1+ι,1+3*ι,2,2+ι,2+2*ι,2+6*ι,3+4*ι,2+11*ι,4]I suggest to leave smoothOver constrained by Integral a, in which case abs is a valid norm. And expose smoothOver' to users as a function, which works for any Euclidean.
The suggestions from the latest review by @Bodigrim have been implemented.
b976bc1
to
6e3cef2
Compare
|
@Bodigrim should be good to go now. |
Math/NumberTheory/SmoothNumbers.hs
Outdated
| -- This function relies on the fact that for any element of a smooth basis @p@ | ||
| -- and any @a@ it is true that @norm (a * p) > norm a@. | ||
| -- This condition is not checked. | ||
| smoothOver'' :: forall a b . (Num a, Ord b) => (a -> b) -> SmoothBasis a -> [a] |
There was a problem hiding this comment.
Wow! This is exciting. Actually, I did not realise that Euclidean constraint is redundant here and Num is sufficient. Let us stick to Num in other function as well.
There was a problem hiding this comment.
existing
What do you mean here?
There was a problem hiding this comment.
@Bodigrim won't dropping the Integral constraint in smoothOver mean abs can no longer be used as norm?
There was a problem hiding this comment.
Let us leave smoothOver with Integral, but make smoothOver' == smoothOver''.
Math/NumberTheory/SmoothNumbers.hs
Outdated
| v' [] = True | ||
| v' (x:xs) = x /= 0 && abs x /= 1 && abs x == x && all (coprime x) xs && v' xs | ||
| v' (x:xs) = x /= 0 && abs x /= 1 && abs x == x && all (E.coprime x) xs && v' xs |
There was a problem hiding this comment.
Let us skip the check for coprimality, which is the main reason for Euclidean constraint. It is enough to check that there are no duplicates. This will have a price: go2 above may encounter duplicates, but it isn't too expensive to deal with them.
There was a problem hiding this comment.
I'm not sure I agree with that. Does it make sense to have {2,4}-smooth numbers, or {2,6}-smooth?
There was a problem hiding this comment.
Wait, I answered my own question. It does make sense to have {2,4}/{2,6}-smooth numbers, it's smoothness over a set. Calling it a basis doesn't make a difference.
Although the comments will need to reflect this change.
There was a problem hiding this comment.
It is debatable, of course. I agree, {2,4}-smooth sounds silly. But {2,6}-smooth makes a sense for me: 2, 4, 6, 8, 12, 16, 24, 32, 36... These are neither {2,3}-smooth numbers, nor even their even subsequence.
There was a problem hiding this comment.
Then I'll also change comments to reflect this.
Math/NumberTheory/SmoothNumbers.hs
Outdated
| where | ||
| mf :: [a] -> a -> Bool | ||
| mf _ 0 = False | ||
| mf [] n = n == 1 -- mf means manually factor |
There was a problem hiding this comment.
Let us relax this check to abs n == 1.
There was a problem hiding this comment.
@Bodigrim good catch. Negative numbers can be smooth.
| go2 a@(ah:at) b@(bh:bt) | ||
| | bh < ah = bh : (go2 a bt) | ||
| | norm bh < norm ah = bh : (go2 a bt) | ||
| | otherwise = ah : (go2 at b) -- no possibility of duplicates |
There was a problem hiding this comment.
Now it is worth to check for duplicates. Otherwise some smooth numbers may repeat. For example,
> take 10 $ smoothOver $ Data.Maybe.fromJust $ fromList [2,4]
[1,2,4,4,8,8,16,16,16,32](It is also worth to add a test IMHO)
There was a problem hiding this comment.
@Bodigrim I really don't like the thought of using nub ∈ O(N²) here... I think it's worth a refactor to use a Set inside this loop.
There was a problem hiding this comment.
Nevermind, I spoke too soon. It was a quick fix in the line below this one, just another guard in the pattern match.
Because smooth numbers tend to grow very quickly, the tests checking that `smoothOver` does not generate duplicates for the `Int/Word` types overflow quickly, which creates a lot of zeroes that `nub` then removes, leaving only one. This means the duplicate-free list is different than the original, causing the test to fail. As such, the tests are only run for `Natural/Integer` now.
|
Great work! Thanks, merged. |
Closes #137.