<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
Philippe, <br>
<br>
Thanks for your review. See replies to specific points below.<br>
<br>
On 8/28/2010 10:24 AM, Philippe Sigaud wrote:
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite"><br>
<br>
<div class="gmail_quote">On Sat, Aug 28, 2010 at 02:48, David
Simcha <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:dsimcha@gmail.com">dsimcha@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
I've cleaned up/fixed the bit rot in my Rational library.
It's available for review at: <a moz-do-not-send="true"
href="http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d?rev=785"
target="_blank">http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d?rev=785</a>
.<br>
</blockquote>
<div><br>
Let's give it a try.<br>
First thing, it's not your faut, but:<br>
<br>
import rational;<br>
void main()<br>
{<br>
auto r = rational(1,2); // error, finds module rational
and not rational.rational<br>
}<br>
<br>
This is going to be a PITA...<br>
<br>
Let say from nom on, I prefix everything with rational.*<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
auto r = rational(1,2);<br>
r = 1; // do not work.<br>
<br>
Maybe you could add opAssign from integer numbers?<br>
</div>
</div>
</blockquote>
<br>
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:
<a class="moz-txt-link-freetext" href="http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d">http://dsource.org/projects/scrapple/browser/trunk/rational/rational.d</a><br>
<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>
<br>
Next, 1/0. OK, it's correctly dealt with (Integer Divide By
Zero Error)<br>
But 0/1 botches. In fact, any rational that's zero creates a
DBZ error. r-= r, etc.<br>
I guess it's a pb in simplify (454-455) or gcf (638), you
should test for a zero numerator or divisor<br>
</div>
</div>
</blockquote>
<br>
I can't believe I forgot to test this edge case. Fixed by simply
testing for zero as a special case in simplify().<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>
<br>
Now, for the code itself:<br>
line 35: Maybe you should add %, %=, they are part of what I
consider 'integer like'. You use them in gcf anyway<br>
</div>
</div>
</blockquote>
<br>
Good point. Done. Also added comparison.<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
As an aside, isIntegerLike, the equivalent isFloatingPointLike
and defineOperators!( some string tuple) could be interesting
additions to std.traits. <br>
I don't know what FP-like would be, maybe test for values
between 0 and 1, and test for NaN.<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
75: if you make it public, you should add a template
constraint if(isIntegral!I1 && isIntegral!i2). The
same for gcf at line 640<br>
</div>
</div>
</blockquote>
<br>
Good point. Fixed.<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>
76: why * instead of +?<br>
</div>
</div>
</blockquote>
<br>
No particular reason. Any reasonable type should return the same
type for either one. I can't see why it would matter either way.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
303: swap in invert. I like this!<br>
<br>
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.<br>
<br>
559: could toRational be a constructor? auto r = rational(PI,
1e-8);<br>
</div>
</div>
</blockquote>
<br>
I'd rather leave it as a free function for a few reasons:<br>
<br>
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.<br>
<br>
2. IIRC templated constructors are generally buggy, though I can't
remember specifically how they're buggy.<br>
<br>
3. Lower case names are easier to type and read.<br>
<br>
4. It's what I already have written and I'm lazy. :-)<br>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
638, 675: Maybe gcf/lcm could become part of std.numerics?<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
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.<br>
<br>
<br>
Some possible additions: <br>
* an integerPart function or property (returning an Int) or
casting as an Int directly, 22/7 -> 3<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>* a fractional part function (ie 22/7 is ~3.14 -> 0.14)
as a FP<br>
</div>
</div>
</blockquote>
<br>
Ditto.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>* Accordingly, floor/ceil functions could be interesting,
unless you think the user should use
std.math.floor(cast(real)rat), which I could understand.<br>
</div>
</div>
</blockquote>
<br>
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. <br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>
* Addendum (probably not a good idea): you can have infinity
and NaN-like values with 1/0, -1/0 and 0/0...<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
***<br>
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.<br>
***<br>
<br>
About converting fixed-width integer-like types, maybe these
should provide .min and .max properties?<br>
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)<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
If anyone has any better ideas, or considers this too severe a
limitation, let me know.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>
<br>
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!<br>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:AANLkTimX=ivWAgfmnq=LwSFLVtpq9_2L_i+5gNobqCbx@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
<br>
*goes back to cleaning his algebraic data types and type
mimicry code*<br>
<br>
</div>
</div>
</blockquote>
<br>
Great. I look forward to when this code is ready.<br>
<br>
</body>
</html>