The Inbox: Kernel-mtf.924.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: Kernel-mtf.924.mcz

commits-2
A new version of Kernel was added to project The Inbox:
http://source.squeak.org/inbox/Kernel-mtf.924.mcz

==================== Summary ====================

Name: Kernel-mtf.924
Author: mtf
Time: 2 May 2015, 12:26:05.675 pm
UUID: 7f688860-5202-4c0d-8ece-8723fe7317d6
Ancestors: Kernel-nice.923

Copied Number >> #round: from Pharo. One of 3 methods needed to make the Artefact pdf library work on squeak: https://sites.google.com/site/artefactpdf/

=============== Diff against Kernel-nice.923 ===============

Item was added:
+ ----- Method: Float>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+         "only leave a fixed amount of decimal"
+         "10.12345 round: 2 => 10.12"
+        
+         | v |
+         v := 10 raisedTo: numberOfWishedDecimal.
+         ^ ((self * v) rounded / v) asFloat
+ !

Item was added:
+ ----- Method: Fraction>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+ ^self asFloat round: numberOfWishedDecimal!

Item was added:
+ ----- Method: Integer>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+ ^self!

Item was added:
+ ----- Method: Number>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+ self subclassResponsibility!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-mtf.924.mcz

Nicolas Cellier


2015-05-02 18:27 GMT+02:00 <[hidden email]>:
A new version of Kernel was added to project The Inbox:
http://source.squeak.org/inbox/Kernel-mtf.924.mcz

==================== Summary ====================

Name: Kernel-mtf.924
Author: mtf
Time: 2 May 2015, 12:26:05.675 pm
UUID: 7f688860-5202-4c0d-8ece-8723fe7317d6
Ancestors: Kernel-nice.923

Copied Number >> #round: from Pharo. One of 3 methods needed to make the Artefact pdf library work on squeak: https://sites.google.com/site/artefactpdf/

=============== Diff against Kernel-nice.923 ===============

Item was added:
+ ----- Method: Float>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+         "only leave a fixed amount of decimal"
+         "10.12345 round: 2 => 10.12"
+
+         | v |
+         v := 10 raisedTo: numberOfWishedDecimal.
+         ^ ((self * v) rounded / v) asFloat
+ !


self assert: (Float fmax round: 2) = Float fmax.

It's probably not very important for artefact, but if this method has a wider usage than artefact, then it should better be robust to overflow...
 
Item was added:
+ ----- Method: Fraction>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+       ^self asFloat round: numberOfWishedDecimal!


Transforming exact arithmetic into inexact Float sounds like heresy to me ;)
That's a questionable choice.
Why this cult to Float would be necessary here?
 
Item was added:
+ ----- Method: Integer>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+       ^self!

Item was added:
+ ----- Method: Number>>round: (in category 'truncation and round off') -----
+ round: numberOfWishedDecimal
+       self subclassResponsibility!


If it's subclass responsibility, then it must be implemented in ScaledDecimal.
This is not necessary in Pharo, because ScaledDecimal has been moved under Fraction, but it is in Squeak.
I'd rather see Float implementation moved up (without the asFloat) and Float invoking super asFloat...

I didn't check artefact, but I suspect that the usage is just to printShowingMaxDecimals: numberOfWishedDecimal, which would make the whole method moot.

Nicolas


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-mtf.924.mcz

Levente Uzonyi-2
On Sat, 2 May 2015, Nicolas Cellier wrote:

>
>
> 2015-05-02 18:27 GMT+02:00 <[hidden email]>:
>       A new version of Kernel was added to project The Inbox:
>       http://source.squeak.org/inbox/Kernel-mtf.924.mcz
>
>       ==================== Summary ====================
>
>       Name: Kernel-mtf.924
>       Author: mtf
>       Time: 2 May 2015, 12:26:05.675 pm
>       UUID: 7f688860-5202-4c0d-8ece-8723fe7317d6
>       Ancestors: Kernel-nice.923
>
>       Copied Number >> #round: from Pharo. One of 3 methods needed to make the Artefact pdf library work on squeak:
>       https://sites.google.com/site/artefactpdf/
>
>       =============== Diff against Kernel-nice.923 ===============
>
>       Item was added:
>       + ----- Method: Float>>round: (in category 'truncation and round off') -----
>       + round: numberOfWishedDecimal
>       +         "only leave a fixed amount of decimal"
>       +         "10.12345 round: 2 => 10.12"
>       +
>       +         | v |
>       +         v := 10 raisedTo: numberOfWishedDecimal.
>       +         ^ ((self * v) rounded / v) asFloat
>       + !
>
>
> self assert: (Float fmax round: 2) = Float fmax.
>
> It's probably not very important for artefact, but if this method has a wider usage than artefact, then it should better be robust to overflow...
>  
>       Item was added:
>       + ----- Method: Fraction>>round: (in category 'truncation and round off') -----
>       + round: numberOfWishedDecimal
>       +       ^self asFloat round: numberOfWishedDecimal!
>
>
> Transforming exact arithmetic into inexact Float sounds like heresy to me ;)
> That's a questionable choice.
> Why this cult to Float would be necessary here?
>  
>       Item was added:
>       + ----- Method: Integer>>round: (in category 'truncation and round off') -----
>       + round: numberOfWishedDecimal
>       +       ^self!
>
>       Item was added:
>       + ----- Method: Number>>round: (in category 'truncation and round off') -----
>       + round: numberOfWishedDecimal
>       +       self subclassResponsibility!
>
>
> If it's subclass responsibility, then it must be implemented in ScaledDecimal.
> This is not necessary in Pharo, because ScaledDecimal has been moved under Fraction, but it is in Squeak.
> I'd rather see Float implementation moved up (without the asFloat) and Float invoking super asFloat...
>
> I didn't check artefact, but I suspect that the usage is just to printShowingMaxDecimals: numberOfWishedDecimal, which would make the whole method
> moot.
The method was probably written by someone who's not aware of #roundTo:.
You're right about the printing thing in Artefact. I replaced #round: with
#printOn:maxDecimalPlaces: in my image.

Levente

>
> Nicolas
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-mtf.924.mcz

Nicolas Cellier


2015-05-02 22:32 GMT+02:00 Levente Uzonyi <[hidden email]>:
On Sat, 2 May 2015, Nicolas Cellier wrote:



2015-05-02 18:27 GMT+02:00 <[hidden email]>:
      A new version of Kernel was added to project The Inbox:
      http://source.squeak.org/inbox/Kernel-mtf.924.mcz

      ==================== Summary ====================

      Name: Kernel-mtf.924
      Author: mtf
      Time: 2 May 2015, 12:26:05.675 pm
      UUID: 7f688860-5202-4c0d-8ece-8723fe7317d6
      Ancestors: Kernel-nice.923

      Copied Number >> #round: from Pharo. One of 3 methods needed to make the Artefact pdf library work on squeak:
      https://sites.google.com/site/artefactpdf/

      =============== Diff against Kernel-nice.923 ===============

      Item was added:
      + ----- Method: Float>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +         "only leave a fixed amount of decimal"
      +         "10.12345 round: 2 => 10.12"
      +
      +         | v |
      +         v := 10 raisedTo: numberOfWishedDecimal.
      +         ^ ((self * v) rounded / v) asFloat
      + !


self assert: (Float fmax round: 2) = Float fmax.

It's probably not very important for artefact, but if this method has a wider usage than artefact, then it should better be robust to overflow...
 
      Item was added:
      + ----- Method: Fraction>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       ^self asFloat round: numberOfWishedDecimal!


Transforming exact arithmetic into inexact Float sounds like heresy to me ;)
That's a questionable choice.
Why this cult to Float would be necessary here?
 
      Item was added:
      + ----- Method: Integer>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       ^self!

      Item was added:
      + ----- Method: Number>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       self subclassResponsibility!


If it's subclass responsibility, then it must be implemented in ScaledDecimal.
This is not necessary in Pharo, because ScaledDecimal has been moved under Fraction, but it is in Squeak.
I'd rather see Float implementation moved up (without the asFloat) and Float invoking super asFloat...

I didn't check artefact, but I suspect that the usage is just to printShowingMaxDecimals: numberOfWishedDecimal, which would make the whole method
moot.

The method was probably written by someone who's not aware of #roundTo:.`

No, this is because 'x roundTo: (10 raisedTo: n) reciprocal' is a bit worse than '(x * (10 raisedTo: n) asFloat) rounded / (10 raisedTo: n) asFloat'
Indeed, in the first one you perform one more inexact operation (1/100 ~= 0.01).

But anyway, these are vain efforts, because #round: in Pharo still cumulates several inexact operations and thus will sometimes fail to deliver the correct answer.

For example:
    self assert: 1.0005 < (5/10000+1) ==> ((1.0005 round: 3) = 1).
alas, it answers 1.001 and differs from printShowingMaxDecimalPlaces: 3...
Definitely not top quality.

I think Pharo did not integrate printShowingMaxDecimalPlaces: so that probably explains the mistake.
Or maybe they were influenced by Python round, but should have checked how it is implemented:

double round(double x) { double absx, y; absx = fabs(x); y = floor(absx); if (absx - y >= 0.5) y += 1.0; return copysign(y, x); }

That should translate easily in Smalltalk if REALLY necessary...


You're right about the printing thing in Artefact. I replaced #round: with #printOn:maxDecimalPlaces: in my image.


Easy to guess, the only other possible use I can think of, is naive monetary apps; that makes two reasons to ban the message ;)



Levente


Nicolas







Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-mtf.924.mcz

Nicolas Cellier


2015-05-02 23:04 GMT+02:00 Nicolas Cellier <[hidden email]>:


2015-05-02 22:32 GMT+02:00 Levente Uzonyi <[hidden email]>:
On Sat, 2 May 2015, Nicolas Cellier wrote:



2015-05-02 18:27 GMT+02:00 <[hidden email]>:
      A new version of Kernel was added to project The Inbox:
      http://source.squeak.org/inbox/Kernel-mtf.924.mcz

      ==================== Summary ====================

      Name: Kernel-mtf.924
      Author: mtf
      Time: 2 May 2015, 12:26:05.675 pm
      UUID: 7f688860-5202-4c0d-8ece-8723fe7317d6
      Ancestors: Kernel-nice.923

      Copied Number >> #round: from Pharo. One of 3 methods needed to make the Artefact pdf library work on squeak:
      https://sites.google.com/site/artefactpdf/

      =============== Diff against Kernel-nice.923 ===============

      Item was added:
      + ----- Method: Float>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +         "only leave a fixed amount of decimal"
      +         "10.12345 round: 2 => 10.12"
      +
      +         | v |
      +         v := 10 raisedTo: numberOfWishedDecimal.
      +         ^ ((self * v) rounded / v) asFloat
      + !


self assert: (Float fmax round: 2) = Float fmax.

It's probably not very important for artefact, but if this method has a wider usage than artefact, then it should better be robust to overflow...
 
      Item was added:
      + ----- Method: Fraction>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       ^self asFloat round: numberOfWishedDecimal!


Transforming exact arithmetic into inexact Float sounds like heresy to me ;)
That's a questionable choice.
Why this cult to Float would be necessary here?
 
      Item was added:
      + ----- Method: Integer>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       ^self!

      Item was added:
      + ----- Method: Number>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       self subclassResponsibility!


If it's subclass responsibility, then it must be implemented in ScaledDecimal.
This is not necessary in Pharo, because ScaledDecimal has been moved under Fraction, but it is in Squeak.
I'd rather see Float implementation moved up (without the asFloat) and Float invoking super asFloat...

I didn't check artefact, but I suspect that the usage is just to printShowingMaxDecimals: numberOfWishedDecimal, which would make the whole method
moot.

The method was probably written by someone who's not aware of #roundTo:.`

No, this is because 'x roundTo: (10 raisedTo: n) reciprocal' is a bit worse than '(x * (10 raisedTo: n) asFloat) rounded / (10 raisedTo: n) asFloat'
Indeed, in the first one you perform one more inexact operation (1/100 ~= 0.01).

But anyway, these are vain efforts, because #round: in Pharo still cumulates several inexact operations and thus will sometimes fail to deliver the correct answer.

For example:
    self assert: 1.0005 < (5/10000+1) ==> ((1.0005 round: 3) = 1).
alas, it answers 1.001 and differs from printShowingMaxDecimalPlaces: 3...
Definitely not top quality.

I think Pharo did not integrate printShowingMaxDecimalPlaces: so that probably explains the mistake.
Or maybe they were influenced by Python round, but should have checked how it is implemented:

double round(double x) { double absx, y; absx = fabs(x); y = floor(absx); if (absx - y >= 0.5) y += 1.0; return copysign(y, x); }

Oups, that's round without argument...
 

That should translate easily in Smalltalk if REALLY necessary...


You're right about the printing thing in Artefact. I replaced #round: with #printOn:maxDecimalPlaces: in my image.


Easy to guess, the only other possible use I can think of, is naive monetary apps; that makes two reasons to ban the message ;)



Levente


Nicolas








Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Kernel-mtf.924.mcz

Nicolas Cellier


2015-05-02 23:10 GMT+02:00 Nicolas Cellier <[hidden email]>:


2015-05-02 23:04 GMT+02:00 Nicolas Cellier <[hidden email]>:


2015-05-02 22:32 GMT+02:00 Levente Uzonyi <[hidden email]>:
On Sat, 2 May 2015, Nicolas Cellier wrote:



2015-05-02 18:27 GMT+02:00 <[hidden email]>:
      A new version of Kernel was added to project The Inbox:
      http://source.squeak.org/inbox/Kernel-mtf.924.mcz

      ==================== Summary ====================

      Name: Kernel-mtf.924
      Author: mtf
      Time: 2 May 2015, 12:26:05.675 pm
      UUID: 7f688860-5202-4c0d-8ece-8723fe7317d6
      Ancestors: Kernel-nice.923

      Copied Number >> #round: from Pharo. One of 3 methods needed to make the Artefact pdf library work on squeak:
      https://sites.google.com/site/artefactpdf/

      =============== Diff against Kernel-nice.923 ===============

      Item was added:
      + ----- Method: Float>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +         "only leave a fixed amount of decimal"
      +         "10.12345 round: 2 => 10.12"
      +
      +         | v |
      +         v := 10 raisedTo: numberOfWishedDecimal.
      +         ^ ((self * v) rounded / v) asFloat
      + !


self assert: (Float fmax round: 2) = Float fmax.

It's probably not very important for artefact, but if this method has a wider usage than artefact, then it should better be robust to overflow...
 
      Item was added:
      + ----- Method: Fraction>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       ^self asFloat round: numberOfWishedDecimal!


Transforming exact arithmetic into inexact Float sounds like heresy to me ;)
That's a questionable choice.
Why this cult to Float would be necessary here?
 
      Item was added:
      + ----- Method: Integer>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       ^self!

      Item was added:
      + ----- Method: Number>>round: (in category 'truncation and round off') -----
      + round: numberOfWishedDecimal
      +       self subclassResponsibility!


If it's subclass responsibility, then it must be implemented in ScaledDecimal.
This is not necessary in Pharo, because ScaledDecimal has been moved under Fraction, but it is in Squeak.
I'd rather see Float implementation moved up (without the asFloat) and Float invoking super asFloat...

I didn't check artefact, but I suspect that the usage is just to printShowingMaxDecimals: numberOfWishedDecimal, which would make the whole method
moot.

The method was probably written by someone who's not aware of #roundTo:.`

No, this is because 'x roundTo: (10 raisedTo: n) reciprocal' is a bit worse than '(x * (10 raisedTo: n) asFloat) rounded / (10 raisedTo: n) asFloat'
Indeed, in the first one you perform one more inexact operation (1/100 ~= 0.01).

But anyway, these are vain efforts, because #round: in Pharo still cumulates several inexact operations and thus will sometimes fail to deliver the correct answer.

For example:
    self assert: 1.0005 < (5/10000+1) ==> ((1.0005 round: 3) = 1).
alas, it answers 1.001 and differs from printShowingMaxDecimalPlaces: 3...
Definitely not top quality.

I think Pharo did not integrate printShowingMaxDecimalPlaces: so that probably explains the mistake.
Or maybe they were influenced by Python round, but should have checked how it is implemented:

double round(double x) { double absx, y; absx = fabs(x); y = floor(absx); if (absx - y >= 0.5) y += 1.0; return copysign(y, x); }

Oups, that's round without argument...
 

And it is using dtoa, that is equivalent to ^(Float readFrom: (self printShowingDecimalPlaces: n) readStream)
 

That should translate easily in Smalltalk if REALLY necessary...


You're right about the printing thing in Artefact. I replaced #round: with #printOn:maxDecimalPlaces: in my image.


Easy to guess, the only other possible use I can think of, is naive monetary apps; that makes two reasons to ban the message ;)



Levente


Nicolas