dsimcha / Rational

std.rational Phobos candidate module
6 stars 2 forks source link

Status of std.rational #1

Open WebDrake opened 11 years ago

WebDrake commented 11 years ago

Your std.rational prototype has come up in a recent D discussion: http://forum.dlang.org/thread/mailman.1747.1380128355.1719.digitalmars-d@puremagic.com

I'm curious to know if there's a particular reason why it was never submitted for inclusion in Phobos proper, if there are any outstanding issues you intended to work on but never had time for. There is clearly still interest, so it'd be great if this could be finished up -- some of us might be willing to take it and run with it if you don't have the time yourself, but it seems only polite to ask you first :-)

WebDrake commented 11 years ago

Just as an FYI, I've started a fork here: https://github.com/WebDrake/Rational

Currently the only difference is some cleanup to bring things in line with current Phobos style guide. I'm impressed by the 94% code coverage of the unittests, very good job!

I'll be doing some more in-depth review of the code in the next week, including trying to sort out some of the bug workarounds in the current code. If you'd like to give input, that'd be fantastic -- if not, thanks for giving us what seems like a really well-written module that probably requires very little work to get it into Phobos.

dsimcha commented 11 years ago

Submission to Phobos proper was discussed a long time ago. At the time, Don Clugston thought that the handling of GCF for BigInt needed improvements that would involve coupling computation of BigInt GCF more closely to the BigInt implementation. Since we never had a concrete plan for doing this, it kind of got pushed to the back burner. I'd be happy to see it revived if these issues have been resolved (I haven't been following D as closely lately) or if there's sufficient demand for machine sized integer rational arithmetic.

I'll review the patches sent to me and catch up on the forum posts tonight. This completely flew under my radar until now. I'd be willing to put some effort into moving this forward.

On Fri, Sep 27, 2013 at 10:50 AM, Joseph Rushton Wakeling < notifications@github.com> wrote:

Just as an FYI, I've started a fork here: https://github.com/WebDrake/Rational

Currently the only difference is some cleanup to bring things in line with current Phobos style guide. I'm impressed by the 94% code coverage of the unittests, very good job!

I'll be doing some more in-depth review of the code in the next week, including trying to sort out some of the bug workarounds in the current code. If you'd like to give input, that'd be fantastic -- if not, thanks for giving us what seems like a really well-written module that probably requires very little work to get it into Phobos.

— Reply to this email directly or view it on GitHubhttps://github.com/dsimcha/Rational/issues/1#issuecomment-25250568 .

WebDrake commented 11 years ago

That would be great -- I look forward to moving this forward!

Because we didn't know whether or not to expect your input I opened up issue tracking on my fork -- we identified a number of issues there including the GCF (it has a bug where gcf(0, n) or gcf(n, 0) will fail). You may want to have a read through there: https://github.com/WebDrake/Rational/issues

I'd assumed based on the various template compile-time checks you defined that you weren't simply intending for it to work with std.bigint but also with arbitrary integer type implementations (I wondered if you were thinking of e.g. GMP?). Is this a concern or is it sufficient that std.rational works with the available built-in integer types plus BigInt?

WebDrake commented 11 years ago

I've fixed the GCF bug in https://github.com/WebDrake/Rational/commit/6667c835af896325ccd588c3f87f9c188f359b2e although I feel it's an imperfect solution (it doesn't work with const/immutable BigInt).

dsimcha commented 11 years ago

I've looked over everything. Thanks for your help getting this in! I put it up for review once before, but other than what I've mentioned previously I don't even remember the details of why it ended up not being included in Phobos. The only thing I see that might need to be done besides what you've mentioned is adding inout declarations to some places to make it const-correct. (inout didn't exist when I wrote version 1 of this thing.)

Is there anything else I can do to help out? I'm more busy than I was back when I wrote this, but I'm sure I could find time to give this an extra little push to get it into Phobos.

On Mon, Sep 30, 2013 at 2:07 PM, Joseph Rushton Wakeling < notifications@github.com> wrote:

I've fixed the GCF bug in WebDrake@6667c83https://github.com/WebDrake/Rational/commit/6667c835af896325ccd588c3f87f9c188f359b2ealthough I feel it's an imperfect solution (it doesn't work with const/immutable BigInt).

— Reply to this email directly or view it on GitHubhttps://github.com/dsimcha/Rational/issues/1#issuecomment-25387587 .

WebDrake commented 11 years ago

It's OK, I anticipated that your free time would be limited if you had any at all to give to this, so anything you can offer is a kindness. I think the most helpful thing is simply to be able to provide feedback when I have questions about the design and how it was meant to fit with other aspects of Phobos.

So, with that in mind, concrete questions :-)

  1. Was there any model for std.rational (e.g. Boost.Rational) or is this a completely clean-room rational number implementation?
  2. Am I right that std.rational is designed to work not just with built-in integers and std.bigint but also with arbitrary alternative integer implementations?
  3. What's wrong with existing std.math.abs that iAbs is necessary? Looking inside the code I don't see a strong difference between what the two of them do and how they do it, but maybe I've missed a subtlety.
  4. Was toRational intended to hook into std.conv.to in any way, shape or form? If so, how was that meant to work?
  5. Were CommonRational and/or CommonInteger meant to hook into CommonType?
dsimcha commented 11 years ago

On Tue, Oct 1, 2013 at 4:42 AM, Joseph Rushton Wakeling < notifications@github.com> wrote:

It's OK, I anticipated that your free time would be limited if you had any at all to give to this, so anything you can offer is a kindness. I think the most helpful thing is simply to be able to provide feedback when I have questions about the design and how it was meant to fit with other aspects of Phobos.

So, with that in mind, concrete questions :-)

1.

Was there any model for std.rational (e.g. Boost.Rational) or is this a completely clean-room rational number implementation?

It's loosely based on the Maxima computer algebra system.

1.

Am I right that std.rational is designed to work not just with built-in integers and std.bigint but also with arbitrary alternative integer implementations?

Yes, that's correct, as long as they overload the relevant operators correctly.

1.

What's wrong with existing std.math.abs that iAbs is necessary? Looking inside the code I don't see a strong difference between what the two of them do and how they do it, but maybe I've missed a subtlety.

IIRC std.math.abs didn't work with BigInt when this code was written. I think it checked for built-in numeric types in a template constraint.

1.

Was toRational intended to hook into std.conv.to in any way, shape or form? If so, how was that meant to work?

No, it wasn't, at least not in the foreseeable future. Maybe in the distant future.

1.

Were CommonRational and/or CommonInteger meant to hook into CommonType?

No. These are much more "specialized" than CommonType, so there's no significant relationship.

— Reply to this email directly or view it on GitHubhttps://github.com/dsimcha/Rational/issues/1#issuecomment-25433645 .

WebDrake commented 11 years ago

On 02/10/13 05:01, David Simcha wrote:

Was there any model for std.rational (e.g. Boost.Rational) or is this a completely clean-room rational number implementation?

It's loosely based on the Maxima computer algebra system.

Ahh, OK. I'm not familiar with that except by name. :-( Is there anything in particular that you feel is important about that relationship? It looks to me like most public API functions for rationals will be the same or similar no matter what the implementation.

Am I right that std.rational is designed to work not just with built-in integers and std.bigint but also with arbitrary alternative integer implementations?

Yes, that's correct, as long as they overload the relevant operators correctly.

Sure, that's an inevitable and acceptable condition. But it's nice to have this genericity -- I'll put it on the table as something that should be available from as many maths functions as possible.

What's wrong with existing std.math.abs that iAbs is necessary? Looking inside the code I don't see a strong difference between what the two of them do and how they do it, but maybe I've missed a subtlety.

IIRC std.math.abs didn't work with BigInt when this code was written. I think it checked for built-in numeric types in a template constraint.

Current std.math.abs reads:

Num abs(Num)(Num x) @safe pure nothrow
     if (is(typeof(Num.init >= 0)) && is(typeof(-Num.init)) &&
             !(is(Num* : const(ifloat*)) || is(Num* : const(idouble*))
                     || is(Num* : const(ireal*))))
{
     static if (isFloatingPoint!(Num))
         return fabs(x);
     else
         return x>=0 ? x : -x;
}

... so it should be OK BigInt-wise and other integer-wise. So I'm going to switch to that and revisit that decision if anything falls apart.

Was toRational intended to hook into std.conv.to in any way, shape or form? If so, how was that meant to work?

No, it wasn't, at least not in the foreseeable future. Maybe in the distant future.

OK.

Were CommonRational and/or CommonInteger meant to hook into CommonType?

No. These are much more "specialized" than CommonType, so there's no significant relationship.

I learned some useful things about D's generic programming looking inside CommonType in order to understand the details of this, so thanks for the inspiration! :-)

WebDrake commented 11 years ago

Is there a reason why CommonRational is private while CommonInteger is public?

dsimcha commented 11 years ago

Honestly, that's one of those obscure details where your guess is as good as mine by now. I don't remember. On Oct 2, 2013 6:20 AM, "Joseph Rushton Wakeling" notifications@github.com wrote:

Is there a reason why CommonRational is private while CommonInteger is public?

— Reply to this email directly or view it on GitHubhttps://github.com/dsimcha/Rational/issues/1#issuecomment-25528422 .

WebDrake commented 11 years ago

No worries. :-)

One big-picture design issue that worries me is the relationship to floating-point types. Obviously without a BigFloat there are certain limitations to what can be done here. Did you have any particular plans to extend Rational's operators to handle floating-point types, and if so, what were they?

I've noted two issues in particular -- https://github.com/WebDrake/Rational/issues/12 and https://github.com/WebDrake/Rational/issues/13 -- but you may have better insight on this than me. Obviously in principle it should be possible to find an equivalent to any limited-precision floating-point number in the form of a Rational, but in practice the issues look severe -- I'm not sure I want to try something like toRational(1.2345678901234567890123456789e-903) ... :-)

WebDrake commented 11 years ago

I've posted an update to the D forum about this -- it would be great to have your input/feedback here, but don't feel obliged :-) http://forum.dlang.org/thread/mailman.1899.1380723964.1719.digitalmars-d@puremagic.com

WebDrake commented 11 years ago

Further FYI -- at request of forum members, I'm submitting some of the code as standalone Phobos patches that will serve as dependencies for a later std.rational submission.

First patch is here: https://github.com/D-Programming-Language/phobos/pull/1616

I preserved your author credit in the commit :-)