-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement inverseTotient #142
Conversation
@Bodigrim this looks great, I'm taking a look at the paper as well. I don't think we have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you profile the function? I don't think you uploaded the Inverse.hs
file that your executable used.
@Bodigrim found what I was looking for, nevermind. |
It is getting tougher than I thought. First, there must be a bug, taking into account failing tests for |
…licit unit leading coefficient
OK, it seems to be ready. At least I was able to reproduce all results from the paper, including the inverse sum-of-divisors of 10^1000. @rockbmb @b-mehta and anyone else, please review. I expect that there are dark corners, where implementation becomes quite convoluted and obscure for an external observer. Please report such places and I'll add clarifying comments. (Since I am deeply in the context, it is difficult for me to identify them myself.) |
Excellent work. I'll take a look, although I cannot guarantee any interesting insights, this code seems quite profound. |
@Bodigrim I can't reproduce the 10^1000 result, how long did execution take for you? I presume that you didn't do it in ghci, but not even an executable did it for me. |
It took 4139 sec and 576 Mb or RAM. |
Nice work! I'll take a close look in the next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a careful look, but may have let some things slip. Will double-check later today.
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs
Outdated
Show resolved
Hide resolved
{-# SPECIALISE inverseTotient :: Semiring b => (Integer -> b) -> Integer -> b #-} | ||
{-# SPECIALISE inverseTotient :: Semiring b => (Natural -> b) -> Natural -> b #-} | ||
|
||
-- | The inverse 'sigma' 1 function such that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent comments.
zero = MinWord maxBound | ||
one = MinWord 1 | ||
plus (MinWord a) (MinWord b) = MinWord (a `min` b) | ||
times (MinWord a) (MinWord b) = MinWord (a * b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL this how we define one valid semiring for the min
operator, I wasn't quite sure (*)
would work.
@rockbmb thanks for valuable suggestions. |
Took another look, couldn't find anything significant. LGTM, if you don't have anything else to add or fix I suggest you merge it now since making this PR larger will make it more difficult to review, and also should this be rolled back for some reason, larger PRs that touch many things are more difficult to handle. |
newtype MinNatural = MinNatural { unMinNatural :: Maybe Natural } | ||
deriving (Show) | ||
data MinNatural | ||
= MinNatural { unMinNatural :: !Natural } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I didn't give much importance to this before, but why was this Maybe
before, and why change it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that switching to a newly defined sum type allows the non-infinite case to be more strict, since the indirection provided by Just
is removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operations on DirichletSeries
never force values of the underlying Map
: all multiplications and filtering is based solely on keys. So values of the Map
inevitably accumulate huge thunks, unless a) Data.Map.Strict
is used, which triggers weak head normal form (WHNF) evaluation of values, b) datatype for values is strict, meaning that WHNF conicides with a normal form. This is crucial for a decent performance.
Maybe a
type is lazy: forcing WHNF will force only evaluation of Just
constructor, but not the value of a
. On contrary MinNatural
is strict, thanks to bang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! Apologies for the delay.
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs
Outdated
Show resolved
Hide resolved
-- > x `elem` inverseTotient (totient x) | ||
-- | ||
-- The return value is parametrized by a semiring, which allows | ||
-- various applications. E. g., list all preimages: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to include examples of calculating the sum or sum of squares of preimages:
>>> getSum $ inverseTotient Sum 120
4904
>>> getSum $ inverseTotient (Sum . (^2)) 120
1573974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually Sum
is redundant: inverseTotient id 120
works as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! In that case, it might be nice to add a note saying that the semiring on Word is the one given by addition and multiplication, to explain why that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do even something like inverseTotient (sigma 1) 120
%)
For interest, we can also compute the product of preimages:
then
matches the expected value, and counts the number of preimages as a bonus. (It is not hard to check this forms a commutative semiring, and that the map from Another semiring can be given by wrapping
then
|
@b-mehta |
@b-mehta using your |
Absolutely! I was tempted to use the trick from the Chudnovsky tests to generalise to |
This is still an early draft of a future framework for inversion of multiplicative functions, part of #60, implementing an algorithm along the lines of https://arxiv.org/pdf/1401.6054.pdf
It is fascinating how powerful polymorphism is. Here is how we can compute a list of inverses of the totient function at 120. These are all numbers
n
such thattotient n == 120
.What if there are too many inverses to keep in memory and it is enough only count them? Just switch to another type:
Interested in minimal or maximal preimage? No worries.
Two largest preimages? Easy as a pie.