0002118: Integer class >> #primesUpTo: omits the largest prime number when the arg is larger than 25001

(0002955 - 351 - 383 - 383 - 383 - 383 - 383)
sam
10-24-05 16:29

The "- 1" in "limit := max asInteger - 1" seems erroneous. Also, by reading and testing the code, I noticed that in every case it returned the primes up to 11, so I added an extra test for this to call #primesUpTo:do: instead of #largePrimesUpTo:do: in this special case.

I'll attach a proposed changeset (no Monticello setup where I am right now).

(0002967 - 272 - 302 - 302 - 302 - 302 - 302)
andreas
10-25-05 09:15

Depends on your definition of "up to". The one I used was the one from Stream which implements #upTo: by "answering a subcollection from the current access position to the occurrence (if any, but not inclusive) of anObject in the receiver". Notice the "but not inclusive".

(0002968 - 73 - 98 - 98 - 98 - 98 - 98)
sumi
10-25-05 10:20

Integer primesUpTo: 5 " => #(2 3 5) "

seems to include 5, doesn't it?

(0002969 - 52 - 52 - 52 - 52 - 52 - 52)
andreas
10-25-05 11:52

Sure does. But does that invalidate my argument? ;-)

(0002970 - 278 - 290 - 290 - 290 - 290 - 290)
sam
10-25-05 11:54

Andreas: what is *certain* at this time is that the behaviour is bogus and inconsistent. I would propose to apply my CS first then maybe to change the code so that the meaning of upTo is fixed if needed.

Otherwise, nothing will get fixed and the behaviour will stay as it is.

(0002979 - 465 - 465 - 465 - 465 - 465 - 465)
andreas
10-25-05 21:44

All I'm asking for is a justification, any justification for why primesUpTo: should indeed include the argument. You say it's inconsistent, and I agree, but that doesn't necessarily mean it must be changed in the way you propose. I have given an argument for doing it the way I decided to implement the method (consistency with naming conventions in Stream) and if you have an argument for why the method should include the argument (if prime), I'd like to hear it.

(0002982 - 538 - 672 - 672 - 672 - 672 - 672)
sumi
10-26-05 05:49

sam,
Thanks your supplementary explanation.

andreas,
As sam said, I think that the problem is the dependence of behavior of #primesUpTo: upon its argument (larger than 25000 or not). Definition of "up to" is the same of yours, not inclusive of arg (even if it is prime, in this case). I apologize that the title of this report is not proper. The bug is in #primesUpTo:do:, but not in #largePrimesUpTo:do:.

So, the Integer class >> #primesUpTo:do: of

1 to: limit do: [:i | ...

should be

1 to: limit - 1 do: [:i | ...

(0003716 - 544 - 1004 - 1004 - 1004 - 1004 - 1004)
MarcusDenker
02-08-06 20:56

I added tests

testPrimesUpTo

    "upTo: semantics means 'non-inclusive'"
    primes := Integer primesUpTo: 5.
    self assert: primes = #(2 3).

    "this test is green for nn>25000, see #testLargePrimesUpTo"
    nn := 5.
    self deny: (Integer primesUpTo: nn) last = nn.
    self assert: (Integer primesUpTo: nn + 1) last = nn.

and

testLargePrimesUpTo

    | nn |
    nn := (2 raisedTo: 17) - 1.
    self deny: (Integer primesUpTo: nn) last = nn.
    self assert: (Integer primesUpTo: nn + 1) last = nn.


with testPrimesUpTo failing in 3.9a

(0003777 - 5 - 5 - 5 - 5 - 5 - 5)
MarcusDenker
02-13-06 14:41

fixed

Notes
(0002955 - 351 - 383 - 383 - 383 - 383 - 383) sam 10-24-05 16:29	The "- 1" in "limit := max asInteger - 1" seems erroneous. Also, by reading and testing the code, I noticed that in every case it returned the primes up to 11, so I added an extra test for this to call #primesUpTo:do: instead of #largePrimesUpTo:do: in this special case. I'll attach a proposed changeset (no Monticello setup where I am right now).

(0002967 - 272 - 302 - 302 - 302 - 302 - 302) andreas 10-25-05 09:15	Depends on your definition of "up to". The one I used was the one from Stream which implements #upTo: by "answering a subcollection from the current access position to the occurrence (if any, but not inclusive) of anObject in the receiver". Notice the "but not inclusive".

(0002968 - 73 - 98 - 98 - 98 - 98 - 98) sumi 10-25-05 10:20	Integer primesUpTo: 5 " => #(2 3 5) " seems to include 5, doesn't it?

(0002969 - 52 - 52 - 52 - 52 - 52 - 52) andreas 10-25-05 11:52	Sure does. But does that invalidate my argument? ;-)

(0002970 - 278 - 290 - 290 - 290 - 290 - 290) sam 10-25-05 11:54	Andreas: what is certain at this time is that the behaviour is bogus and inconsistent. I would propose to apply my CS first then maybe to change the code so that the meaning of upTo is fixed if needed. Otherwise, nothing will get fixed and the behaviour will stay as it is.

(0002979 - 465 - 465 - 465 - 465 - 465 - 465) andreas 10-25-05 21:44	All I'm asking for is a justification, any justification for why primesUpTo: should indeed include the argument. You say it's inconsistent, and I agree, but that doesn't necessarily mean it must be changed in the way you propose. I have given an argument for doing it the way I decided to implement the method (consistency with naming conventions in Stream) and if you have an argument for why the method should include the argument (if prime), I'd like to hear it.

(0002982 - 538 - 672 - 672 - 672 - 672 - 672) sumi 10-26-05 05:49	sam, Thanks your supplementary explanation. andreas, As sam said, I think that the problem is the dependence of behavior of #primesUpTo: upon its argument (larger than 25000 or not). Definition of "up to" is the same of yours, not inclusive of arg (even if it is prime, in this case). I apologize that the title of this report is not proper. The bug is in #primesUpTo:do:, but not in #largePrimesUpTo:do:. So, the Integer class >> #primesUpTo:do: of 1 to: limit do: [:i \| ... should be 1 to: limit - 1 do: [:i \| ...

(0003716 - 544 - 1004 - 1004 - 1004 - 1004 - 1004) MarcusDenker 02-08-06 20:56	I added tests testPrimesUpTo "upTo: semantics means 'non-inclusive'" primes := Integer primesUpTo: 5. self assert: primes = #(2 3). "this test is green for nn>25000, see #testLargePrimesUpTo" nn := 5. self deny: (Integer primesUpTo: nn) last = nn. self assert: (Integer primesUpTo: nn + 1) last = nn. and testLargePrimesUpTo \| nn \| nn := (2 raisedTo: 17) - 1. self deny: (Integer primesUpTo: nn) last = nn. self assert: (Integer primesUpTo: nn + 1) last = nn. with testPrimesUpTo failing in 3.9a

(0003777 - 5 - 5 - 5 - 5 - 5 - 5) MarcusDenker 02-13-06 14:41	fixed

Issue History
Date Modified	Username	Field	Change
10-24-05 16:10	sumi	New Issue
10-24-05 16:29	sam	Note Added: 0002955
10-24-05 16:30	sam	File Added: Primes0002118.1.cs
10-24-05 21:41	sam	Issue Monitored: sam
10-25-05 09:15	andreas	Note Added: 0002967
10-25-05 10:20	sumi	Note Added: 0002968
10-25-05 11:52	andreas	Note Added: 0002969
10-25-05 11:54	sam	Note Added: 0002970
10-25-05 21:44	andreas	Note Added: 0002979
10-26-05 05:49	sumi	Note Added: 0002982
02-08-06 20:56	MarcusDenker	Note Added: 0003716
02-08-06 21:01	MarcusDenker	File Deleted: Primes0002118.1.cs
02-13-06 14:41	MarcusDenker	Status	new => closed
02-13-06 14:41	MarcusDenker	Note Added: 0003777
02-13-06 14:41	MarcusDenker	Resolution	open => fixed
02-13-06 14:41	MarcusDenker	Fixed in Version	=> 3.9

Relationships