fix: include number/2 divisor in FriendlyNumbers sumDivisors#1904
Open
shaked-shlomo wants to merge 1 commit into
Open
fix: include number/2 divisor in FriendlyNumbers sumDivisors#1904shaked-shlomo wants to merge 1 commit into
shaked-shlomo wants to merge 1 commit into
Conversation
sumDivisors looped 'for (let i = 0; i < number / 2; i++)', which excludes i === number/2 -- a real divisor of every even number -- so the divisor sum (and abundancy index) was wrong for even inputs, e.g. sumDivisors(6) returned 9 instead of 12 and sumDivisors(28) returned 42 instead of 56. That made FriendlyNumbers misclassify pairs (e.g. FriendlyNumbers(15, 20) returned true though their abundancy indices differ). Loop from 1 to number/2 inclusive. Added a test file (the module had none).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1904 +/- ##
==========================================
+ Coverage 85.91% 86.11% +0.19%
==========================================
Files 379 379
Lines 19778 19778
Branches 3016 3024 +8
==========================================
+ Hits 16993 17031 +38
+ Misses 2785 2747 -38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Maths/FriendlyNumbers.js'ssumDivisorsuses an exclusive upper bound:This excludes
i === number / 2, which is a divisor of every even number, so the sum of divisors (and therefore the abundancy indexσ(n)/n) is wrong for even inputs:sumDivisors(6)returns9instead of12sumDivisors(28)returns42instead of56Because the error scales both numbers similarly it sometimes cancels out (the classic
FriendlyNumbers(6, 28)still returnstrueby luck), but it produces wrong classifications in general — e.g.FriendlyNumbers(15, 20)returnstrueeven though σ(15)/15 = 1.6 ≠ σ(20)/20 = 2.1.Fix
Loop from
1tonumber / 2inclusive:(Starting at
1also drops the harmlessnumber / 0 = Infinitycheck the oldi = 0start relied on.)After the fix:
sumDivisors(6) = 12,sumDivisors(28) = 56,FriendlyNumbers(6, 28) = true,FriendlyNumbers(30, 140) = true,FriendlyNumbers(15, 20) = false.The module had no test file, so I added
Maths/test/FriendlyNumbers.test.jscovering friendly pairs, a non-friendly pair, and invalid input.