[phobos] Rational for review

[Philippe Sigaud]

On Sat, Aug 28, 2010 at 02:48, David Simcha <dsimcha at gmail.com> wrote:

>  I've cleaned up/fixed the bit rot in my Rational library.  It's available
> for review at:
> http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d?rev=785.
>

Let's give it a try.
First thing, it's not your faut, but:

import rational;
void main()
{
    auto r = rational(1,2); // error, finds module rational and not
rational.rational
}

This is going to be a PITA...

Let say from nom on, I prefix everything with rational.*

auto r = rational(1,2);
r = 1; // do not work.

Maybe you could add opAssign from integer numbers?

Next, 1/0. OK, it's correctly dealt with (Integer Divide By Zero Error)
But 0/1 botches. In fact, any rational that's zero creates a DBZ error. r-=
r, etc.
I guess it's a pb in simplify (454-455) or gcf (638), you should test for a
zero numerator or divisor

Now, for the code itself:
line 35: Maybe you should add %, %=, they are part of what I consider
'integer like'. You use them in gcf anyway

As an aside, isIntegerLike, the equivalent isFloatingPointLike and
defineOperators!( some string tuple) could be interesting additions to
std.traits.
I don't know what FP-like would be, maybe test for values between 0 and 1,
and test for NaN.

75: if you make it public, you should add a template constraint
if(isIntegral!I1 && isIntegral!i2). The same for gcf at line 640
76: why * instead of +?

303: swap in invert. I like this!

307, 317 (opEqual, opCmp). Wikipedia uses denom1*num2 - denom2*num1 == 0 as
the equivalence relation among rational numbers. Maybe this can be extended
to opCmp ? Ah, you want to avoid overflow, I gather? OK, scratch this.

559: could toRational be a constructor? auto r = rational(PI, 1e-8);

638, 675: Maybe gcf/lcm could become part of std.numerics?

All in all, I like you code very much, it's very clean and readable. It's
even organized the way I'd do it : first templates, then structs, then
functions.


Some possible additions:
* an integerPart function or property (returning an Int) or casting as an
Int directly, 22/7 -> 3
* a fractional part function (ie 22/7 is ~3.14 -> 0.14) as a FP
* Accordingly,  floor/ceil functions could be interesting, unless you think
the user should use std.math.floor(cast(real)rat), which I could understand.
* Addendum (probably not a good idea): you can have infinity and NaN-like
values with 1/0, -1/0 and 0/0...

***
 2.  I couldn't figure out how to handle the case of user-defined
fixed-width integer types in the decimal conversion (opCast) function, so it
just assumes non-builtin integer types are arbitrary precision.  The biggest
problem is lack of a way of introspecting whether the integer is fixed or
arbitrary width and what its fixed width might be if it is, in fact, fixed.
 The secondary problem is that I don't know of a good algorithm (though I'm
sure I could find one if I tried) for converting fixed-width integers to
floating point numbers in software.  Arbitrary precision is much easier.
***

About converting fixed-width integer-like types, maybe these should provide
.min and .max properties?
That way you know the range of possible values and maybe it's then possible
to convert (I didn't think this through so I may be completly mistaken)

Good work David. My only pb is that you write code faster that I can read
it, and don't let me any time to write some myself! It took me a week to
read the recent additions in Phobos!


*goes back to cleaning his algebraic data types and type mimicry code*


Philippe

Philippe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20100828/aa769581/attachment-0001.html>