Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#148] Generalize 'inverseSigma' and 'inverseTotient' #188

Merged
merged 1 commit into from
Mar 3, 2020
Merged

[#148] Generalize 'inverseSigma' and 'inverseTotient' #188

merged 1 commit into from
Mar 3, 2020

Conversation

rockbmb
Copy link
Contributor

@rockbmb rockbmb commented Feb 25, 2020

Closes #148.

@rockbmb rockbmb requested a review from Bodigrim February 25, 2020 14:12
@rockbmb
Copy link
Contributor Author

rockbmb commented Feb 25, 2020

Tests will fail because of some issues with roundtrip tests (may be overflow related in inverseFunction), but the essential is here.

Bodigrim
Bodigrim requested changes Feb 26, 2020
View reviewed changes
Copy link
Owner

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a couple of unit tests as well.

Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated
-> (a -> b)
-> a
-> b
inverseSigmaK k point = invertFunction point (sigmaA k) invSigma
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to generalize invSigma, making it work for any k.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invTotient worked out of the box for inverseJordan, but it seems invSigma will need some work.

Could you clarify the generalization that section 5.2 (of the paper you used to write this) had that you removed because invSigma only needed the k = 1 case?

Copy link
Owner

@Bodigrim Bodigrim Feb 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invTotient works, but mostly by coincidence and doing more than needed. I think ps = mapMaybe (isPrime . (+ 1)) divs should become ps = mapMaybe (\d -> exactRoot k (d + 1) >>= isPrime) divs.

With regards to invSigma it would probably suffice to replace p by p ^ k in conditions in doPrime and pksLarge.

Copy link
Contributor Author

@rockbmb rockbmb Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! That's exactly the hint I needed - this made me understand there are a few other places that need to be changed.
With that in mind, why is this line

, e <- [1 .. intToWord (integerLogBase (toInteger lim) (toInteger d))]

instead of

, e <- [1 .. intToWord (integerLogBase (toInteger lim) (toInteger (d - 1)))]

Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Outdated Show resolved Hide resolved
@rockbmb rockbmb force-pushed the general-inverses branch 4 times, most recently from 14339d7 to 7621884 Compare March 1, 2020 15:11
@rockbmb rockbmb force-pushed the general-inverses branch from 7621884 to 9ee18e8 Compare March 1, 2020 17:03
@rockbmb
Copy link
Contributor Author

rockbmb commented Mar 1, 2020

@Bodigrim added some unit tests from OEIS. Should be good to go.

@rockbmb rockbmb requested a review from Bodigrim March 1, 2020 17:04
@rockbmb rockbmb force-pushed the general-inverses branch from 9ee18e8 to 1bc5e11 Compare March 1, 2020 19:33
Bodigrim
Bodigrim requested changes Mar 1, 2020
View reviewed changes
Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/TestUtils.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Show resolved Hide resolved
Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
@rockbmb rockbmb force-pushed the general-inverses branch 3 times, most recently from 8ff5335 to 5221fe6 Compare March 2, 2020 02:22
@rockbmb rockbmb requested a review from Bodigrim March 2, 2020 02:32
Bodigrim
Bodigrim requested changes Mar 2, 2020
View reviewed changes
Copy link
Owner

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there :) Sorry for so many rounds of review, I'm on the run.

Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
Math/NumberTheory/ArithmeticFunctions/Inverse.hs Outdated Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Show resolved Hide resolved
test-suite/Math/NumberTheory/ArithmeticFunctions/InverseTests.hs Outdated Show resolved Hide resolved
@rockbmb
Copy link
Contributor Author

rockbmb commented Mar 3, 2020

Sorry for so many rounds of review

On the contrary, I appreciate the attention you've been giving this! 👍

@rockbmb rockbmb force-pushed the general-inverses branch from 5221fe6 to 85df1c5 Compare March 3, 2020 00:59
@rockbmb rockbmb force-pushed the general-inverses branch from 85df1c5 to 0ff40e6 Compare March 3, 2020 01:07
@rockbmb rockbmb requested a review from Bodigrim March 3, 2020 14:20
@Bodigrim Bodigrim merged commit 9d5261d into Bodigrim:master Mar 3, 2020
@Bodigrim
Copy link
Owner

Bodigrim commented Mar 3, 2020

Well done!

@rockbmb rockbmb deleted the general-inverses branch March 3, 2020 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bodigrim Bodigrim Bodigrim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inverse the Jordan and the sum-of-powers-of-divisors functions
2 participants
@rockbmb @Bodigrim