Re: [phobos] Rational for review

[David Simcha]

Philippe, Thanks for your review.  See replies to specific points below. On 8/28/2010 10:24 AM, Philippe Sigaud wrote: On Sat, Aug 28, 2010 at 02:48, David Simcha <dsim...@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.* Don't worry about module issues.  As Lars pointed out, the library will not be named rational if/when it's integrated into Phobos.  It will be std.rational or be included in std.numerics. auto r = rational(1,2); r = 1; // do not work. Maybe you could add opAssign from integer numbers? I fixed this right after I sent out the request for comment, because I realized that not allowing it was a bit silly.  Unfortunately, I accidentally sent a link that pointed to a specific revision.  Here's a new link that updates will be reflected in:  http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d 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 I can't believe I forgot to test this edge case.  Fixed by simply testing for zero as a special case in simplify(). 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 Good point.  Done.  Also added comparison. 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. You may be right, but I'd rather wait until more user defined integer and floating point types are around to see exactly what such templates should require. 75: if you make it public, you should add a template constraint if(isIntegral!I1 && isIntegral!i2). The same for gcf at line 640 Good point.  Fixed. 76: why * instead of +? No particular reason.  Any reasonable type should return the same type for either one.  I can't see why it would matter either way. 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); I'd rather leave it as a free function for a few reasons: 1.  In general the convention in Phobos is that non-trivial construction steps for structs are done in free functions.  Even though the reasons for that convention don't apply here, I'd rather follow it unless there's really a good reason to violate it. 2.  IIRC templated constructors are generally buggy, though I can't remember specifically how they're buggy. 3.  Lower case names are easier to type and read. 4.  It's what I already have written and I'm lazy. :-) I don't feel very strongly about this, though.  If you really feel strongly that it should be a constructor, though, and can explain why, then I'm willing to reconsider. 638, 675: Maybe gcf/lcm could become part of std.numerics? I think GCF is already in there.  I rolled my own so that I could tweak it easily to make it do exactly what I want with regard to types, but in the long run you're right that it should be included in std.numerics and whatever changes I made should be merged. 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 This is a great idea, and trivial to implement.  I'll add it as soon as I decide for sure what to do about the user-defined fixed width issue. * a fractional part function (ie 22/7 is ~3.14 -> 0.14) as a FP Ditto. * Accordingly,  floor/ceil functions could be interesting, unless you think the user should use std.math.floor(cast(real)rat), which I could understand. Again, good idea.  Using std.math sucks because a major point of Rational is that it's supposed to work with arbitrary precision, and reals aren't arbitrary precision. * Addendum (probably not a good idea): you can have infinity and NaN-like values with 1/0, -1/0 and 0/0... Probably not.  I really despise NaNs in floating point code because they break fundamental mathematical axioms that appear to be common sense (such as x == x, x * 0 == 0, if !(x <= y) then x > y, and x < y || x == y || x > y).  For example, try making a sorting algorithm or a binary tree with floating point numbers if they may be NaNs.  I'd rather just leave it the way it is, where it throws if the denominator is zero. ***  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) I think what I'm going to do here is just throw an exception iff the denominator has both the highest and lowest order bit set.  This is enough of a corner case in that it will only happen close to the limit of the largest denominator value that can be represented that I don't consider it a major limitation.  It's easily detectable because the numerator will never become bigger than the denominator by bit shifting iff either the numerator is zero or the denominator has its highest order bit set.  If the denominator's lowest order bit isn't set then I can shift the denominator right to make it not have its highest order bit set. If anyone has any better ideas, or considers this too severe a limitation, let me know. 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! This code has been around in some form for almost a year.  It's just that the proposal for a units library made me remember that I've been meaning to clean up all the bit rot, improve it and submit it for inclusion. *goes back to cleaning his algebraic data types and type mimicry code* Great.  I look forward to when this code is ready.

_______________________________________________
phobos mailing list
phobos@puremagic.com
http://lists.puremagic.com/mailman/listinfo/phobos