Apologies. Reading on the phone I only saw the first part of the message. I was talking about isAssignable for now.

Andrei

On 08/28/2010 05:40 PM, David Simcha wrote:
> Yes please what?  isAssignable?  The whole module?  I'm confused.
>
> On 8/28/2010 6:07 PM, Andrei Alexandrescu wrote:
>> Yes please.
>>
>> Sent by shouting through my showerhead.
>>
>> On Aug 28, 2010, at 4:22 PM, David Simcha <dsimcha at gmail.com> wrote:
>>
>>> On 8/28/2010 4:22 PM, Philippe Sigaud wrote:
>>>>
>>>> line 76: isAssignable would be a good addition to std.traits.
>>>
>>> Yeah, will do unless anyone has any objections.
>>>
>>>>
>>>>
>>>>>
>>>>> 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().
>>>>
>>>> Put in some very basic unit tests :-) 0+0==0, 1-1==0, that kind of
>>>> thing :)
>>>
>>> Did that already. They pass.
>>>
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Man, integer-like is beginning to be quite big.
>>>>
>>>> What about adding ++ and -- ?
>>>
>>> I don't need/use them, so there's no real reason to require them.
>>>
>>>>
>>>> Btw, I'm against putting ++ and -- in Rational, but please do add opUnary!"+", opUnary!"-".
>>>
>>> Will do, along with ^^. These were left out as oversights, not for "good" reasons.
>>>
>>>>
>>>>> * a fractional part function (ie 22/7 is ~3.14 -> 0.14) as a FP
>>>>
>>>> Ditto.
>>>>
>>>> Thinking about it some more, maybe the fractional part should return a typeof(this): 22/7 -> 1/7. Not a float.
>>>
>>> Yea, this makes more sense. If you want a float, you can always cast it.
>>>
>>>>
>>>>> * 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.
>>>>
>>>>
>>>> I could argue that floor or ceil are exactly when you get rid of this arbitrary precision, but to answer the question in your next post, I think these should be part of the rational module and be free functions: it makes the module self-supported. And having rational support in std math means it must in turn import the rational module. I'm against this.
>>>>
>>>> In any case, people will probably import both at the same time...
>>>>
>>>> I'm not clear on the subtle nuances between the integer part, the floor and the ceil for positive and negative Rationals, though.
>>>>
>>>> See std.math: ceil, floor, nearbyInt, round, trunc! Awww.
>>>>
>>>> The next question would be: what other free function? I'll add abs(), but not much more. Maybe pow or opBinary!"^^" ?
>>>
>>> opBinary!"^^": Yes. pow(): I don't see the point. Probably everyone
>>> writing generic math code nowadays will use ^^ instead of pow()
>>> because it's less typing and more readable. Then again, pow() would
>>> be trivial to implement in terms of opBinary!"^^".
>>>
>>>> I wouldn't add any trigonometric function.
>>>
>>>> .
>>>> sqrt is on the fence for me. Is there an algorithm to calculate the
>>>> square root of a rational apart from the Babylonian method?
>>>
>>> I'm going to leave trig and sqrt out. To me, the point of rationals is to allow calculations to be exact. Functions where the result can be irrational don't make sense in this context. You'd be better off just casting to real and then calling the std.math functions.
>>>
>>>>
>>>>> ***
>>>>> 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.
>>>>
>>>> What's the pb with having a numerator bigger than the denominator? Sorry, I didn't read the conversion function carefully.
>>>
>>> The way the function works involves bit shifting the numerator left until it's bigger than the denominator. If the denominator has its highest order bit occupied, no amount of shifting will ever make the numerator bigger.
>>>>
>>>> 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.
>>>>
>>>> Seeing that you have time, talent and energy, I'll order a BigDecimal module :-)
>>>
>>> I'd love one, but I haven't the slightest clue how to write one and have several hacking projects that I need to finish/start before even considering it.
>>>
>>> _______________________________________________
>>> phobos mailing list
>>> phobos at puremagic.com
>>> http://lists.puremagic.com/mailman/listinfo/phobos
>> _______________________________________________
>> phobos mailing list
>> phobos at puremagic.com
>> http://lists.puremagic.com/mailman/listinfo/phobos
>>
>
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

This proposal looks almost ready for bringing up to digitalmars.d for a formal review. Essentially you'd need to cross all "t"s and dot all "i"s following suggestions from reviewers on this list, and work on creating a good quality documentation.

I have a few comments and suggestions based on what I just saw at http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d:

* I want us to cover in this same module the case of a rational with a fixed denominator, i.e. a fixed-point type. For example, a financial program might use FixedRational!(long, 100) to operate consistently on cents. (As a curiosity, in the olden days (up until 2002 or so) the stock market operated on FixedRational!(long, 8192).) There are plenty of places in which fixed point values are desirable, and I believe many of them can be expressed as rational values with a compile-time-known denominator.

* The import section should be ordered alphabetically.

* The test isRational suggests that you accept any values for num and denom, but in reality you should constrain that to e.g. isIntegerLike for both.

* isAssignable should be transferred to std.traits

* Isn't CommonType good in place of CommonInteger?

* Can't you consolidate the first two implementations of op"*"?

* Inside opOpAssign, you have the code:

auto divisor = gcf(this.numerator, rhs.denominator);
this.numerator /= divisor;
rhs.denominator /= divisor;

I have the suspicion that an organic routine that computes the gcf and simplifies at the same time could save on some divisions. Don't forget that integral divisions are very, very slow. Same considerations applies for other similar patterns in code.

* Is it worth using Unsigned!T for the denominator throughout? That way there's no more need to worry about fixSigns etc.

* When you swap() in op"/", don't forget to assert that the denominator-to-be is nonzero. Generally I'm not seeing asserts/contracts throughout.

* I have the suspicion that implementing e.g. += in terms of + is actually slower than creating the value directly. Generally on today's architectures you want to minimize mutation, and += is more mutation than +.

* In opCmp, I suspect that converting to double and carrying the computation may be actually cheaper.

* Anyhow, in opCmp instead of

if(lhsNum > rhsNum) {
   return 1;
} else if(lhsNum < rhsNum) {
   return -1;
}
// We've checked for equality already.  If we get to this point,
// there's clearly something wrong.
assert(0);

do this:

assert(lhsNum != rhsNum);
return lhsNum > rhsNum ? 1 : -1;

* Why the use of "this." in some places?

* In opCast!double, can't you use some specialized routines in BigInt instead of rolling your own? Regardless, I suspect BigInt should have a solution for "divide yielding a floating point number" - if not, you may want to discuss with Don moving your implementation into std.bigint.

Again, looking forward to see a formal proposal on digitalmars.d soon!


Andrei

On 8/27/10 7:48 PM, David Simcha 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
> .
>
> Known issues:
>
> 1. Due to bugs in BigInt (mainly 4742 http://d.puremagic.com/issues/show_bug.cgi?id=4742), BigInt instantiations don't play nicely with builtin integer instantiations. For example, this doesn't work:
>
> auto a = rational(BigInt(1), 2) * rational(8, 7);
>
> 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.
>
> 3. There is a small amount of special case ad-hockery to make things
> work with std.bigint.BigInt. I consider these bug workarounds. In
> principle, everything should work with any user defined arbitrary
> precision integer if it overloads everything it's supposed to overload.
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos