www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - std.rational -- update and progress towards review

reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
Hello all,

I thought I'd ask for some guidance here, since this is the first time I'm 
involved in bringing a new Phobos module to review.

If I understand right, "review" means that I present a pull request to Phobos, 
rather than just a standalone piece of code as currently available.  The 
question is how to handle various bits of std.rational that arguably could or 
maybe should be implemented elsewhere in Phobos.  Is there a preference to 
present the module as-is, and have the review process decide what goes where,
or 
is it better to present the pull request including patches against other
modules?

To help with answers to the above, I'll summarize the state of std.rational and 
what I've done so far.

    * std.rational is designed to allow the construction of rational numbers
      based on any of the built-in integer types (it also works with char:-),
      std.bigint.BigInt, or arbitrary user-defined "integer-like" types.

    * Because it doesn't just want to work with built-in integer types, David
      Simcha was forced to define custom versions of various functionality
      found elsewhere in Phobos (or in some cases, perhaps at the time the
      functionality simply was not there).  Some of these have been deleted
      in my recent updates as they are no longer necessary: e.g. today it is
      fine to use std.math.abs, std.traits.isAssignable.  Others remain, and
      the question is how to handle these cases.

    * David defines an isRational template, which is currently very simple
      (it just checks that the type has numerator and denominator properties).
      This presumably is fine to keep in std.rational and should not be moved
      elsewhere.

    * Because std.rational doesn't just want to work with built-in integer types
      it can't rely on the existing isIntegral.  Instead, David defined a new
      template, "isIntegerLike", which checks the operations supported by the
      type and whether they work in an integer-like way.  Could/should this be
      placed in std.traits?  Better to do this in the initial pull or let the
      decision be part of the review process?

    * For similar reasons, CommonType is insufficient and David defined two
      new templates, CommonInteger (which works out an appropriate common
      type for two "integer-like" types) and CommonRational.  The former at
      least looks to me like it should be in std.traits.  Again, better to
      do this in the initial pull request, or make it a decision to take at
      review time?

    * Several Rational-based overrides for std.math functions are defined:
      floor, ceil and round.  I believe it's normal for such type-specific
      overrides to be in the same module as their type?

    * Finally, there are two mathematical functions, gcf (greatest common
factor)
      and lcm (least common multiple) which again are locally defined because
      Phobos' existing std.math.gcd cannot handle BigInts (let alone any other
      user-defined integer type).  These should presumably be sent over to
      std.math but that relies on isIntegerLike being accepted for std.traits,
      or else some alternative type check being in place.

    * (... actually, there is no lcm in std.math in any case.  An oversight:-)

I need to do a careful review of the function attributes (there's a complete 
lack of  safe, pure, nothrow, const, ... in the module, only  property is
used), 
but apart from that I think that the code is now working within the confines of 
its design parameters, and in that sense is ready for review.  I've squashed
the 
only overt bug found, added unittests to confirm this and to check other cases 
(David's were already very extensive so I haven't needed to do much), updated 
and corrected the docs and removed all unnecessary code duplication, and of 
course also brought the code style in line with current Phobos standards.

The remaining open issues 
<https://github.com/WebDrake/Rational/issues?state=open> are all
design-related. 
  Apart from those raised by my above queries, the major one is how rationals 
should relate to floating-point numbers -- e.g. there is currently no opCmp for 
floating-point, meaning this:

     assert(rational(10, 1) == 10);

... will work, but this:

     assert(rational(10, 1) == 10.0);

... will fail to compile.  It's not entirely obvious how to resolve this as 
floating-point vs. rational comparisons risk accidentally creating huge 
temporary BigInt-based rationals ... :-(

Anyway, I'd really value your feedback at this point.

Thanks & best wishes,

     -- Joe
Oct 02 2013
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-10-02 16:25, Joseph Rushton Wakeling wrote:

 Hello all,

 I thought I'd ask for some guidance here, since this is the first time
 I'm involved in bringing a new Phobos module to review.

 If I understand right, "review" means that I present a pull request to
 Phobos, rather than just a standalone piece of code as currently
 available.


A pull request shouldn't be made until the review is done and the module 
has been voted for acceptance. See:

http://wiki.dlang.org/Review/Process

Usually you'll have a fork of Phobos with the changes/new module. Then 
post here, something like: "Request for review of std.<module name>".

-- 
/Jacob Carlborg
Oct 02 2013
parent reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 02/10/13 20:27, Jacob Carlborg wrote:

 A pull request shouldn't be made until the review is done and the module has
 been voted for acceptance. See:

 http://wiki.dlang.org/Review/Process

 Usually you'll have a fork of Phobos with the changes/new module. Then post
 here, something like: "Request for review of std.<module name>".


Ahh, OK -- thanks for clarifying.  But either way, what I meant is: the module 
needs to be provided for review in a way that is diff-able against current 
Phobos, no?
Oct 03 2013
parent reply "Dicebot" <public dicebot.lv> writes:
On Thursday, 3 October 2013 at 10:51:22 UTC, Joseph Rushton 
Wakeling wrote:

 Ahh, OK -- thanks for clarifying.  But either way, what I meant 
 is: the module needs to be provided for review in a way that is 
 diff-able against current Phobos, no?


Yes. Common approach is to fork phobos in private repository and 
integrate proposed code there so that it will be available for 
easy review. Doing actual pull request will only distract Phobos 
devs and it not really needed.
Oct 03 2013
parent reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 03/10/13 13:00, Dicebot wrote:

 Yes. Common approach is to fork phobos in private repository and integrate
 proposed code there so that it will be available for easy review. Doing actual
 pull request will only distract Phobos devs and it not really needed.


As the one who'd be managing the review process, do you have any preferences
for 
how I handle the issues I raised above?  Do you think it'd be better to parcel 
the potentially broadly useful code out across different Phobos modules as 
suggested or let the review process decide what goes where?
Oct 03 2013
parent reply "Dicebot" <public dicebot.lv> writes:
On Thursday, 3 October 2013 at 11:14:26 UTC, Joseph Rushton 
Wakeling wrote:

 As the one who'd be managing the review process, do you have 
 any preferences for how I handle the issues I raised above?  Do 
 you think it'd be better to parcel the potentially broadly 
 useful code out across different Phobos modules as suggested or 
 let the review process decide what goes where?


My role as review manager does not matter here. Responsibility of 
review manager is to collect opinions and information from 
community, he does not have any authority of his own.

Remembering std.serialization discussion though, it felt like 
people are more in favor of merging generic functionality into 
matching modules instead of keeping everything self-contained.

You can also just do pull requests for smaller parts without any 
review if they are decoupled from std.rational - but be warned 
that waiting until those are reviewed and merged by Phobos devs 
may take a considerable time ;)

In the end it is your own judgment. There are no strict rules and 
it is always good to see a module with personality behind ;)
Oct 03 2013
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-10-03 13:31, Dicebot wrote:


 Remembering std.serialization discussion though, it felt like people are
 more in favor of merging generic functionality into matching modules
 instead of keeping everything self-contained.


I got the opposite feeling. That it's generally better for utility 
functions, that could be useful for the rest of Phobos, be kept private. 
Then we can later extract those functions if they're considered useful.

The more public API the harder it will become to get the changes 
accepted. All the bikeshedding about where to put the functions and what 
to name them.

-- 
/Jacob Carlborg
Oct 03 2013
parent reply "Dicebot" <public dicebot.lv> writes:
On Thursday, 3 October 2013 at 11:47:50 UTC, Jacob Carlborg wrote:

 On 2013-10-03 13:31, Dicebot wrote:


 Remembering std.serialization discussion though, it felt like 
 people are
 more in favor of merging generic functionality into matching 
 modules
 instead of keeping everything self-contained.


 I got the opposite feeling. That it's generally better for 
 utility functions, that could be useful for the rest of Phobos, 
 be kept private. Then we can later extract those functions if 
 they're considered useful.

 The more public API the harder it will become to get the 
 changes accepted. All the bikeshedding about where to put the 
 functions and what to name them.


That is a bit frustrating part - we (reviewers) kind of want 
stuff cleanly separated between modules for further re-usage but 
at the same time we will make your life hell when reviewing how 
exactly this stuff needs to be incorporated :P
Oct 03 2013
parent reply Jacob Carlborg <doob me.com> writes:
On 2013-10-03 16:36, Dicebot wrote:


 That is a bit frustrating part - we (reviewers) kind of want stuff
 cleanly separated between modules for further re-usage but at the same
 time we will make your life hell when reviewing how exactly this stuff
 needs to be incorporated :P


Every time I asked that some functionality should be place somewhere 
else because it's generally useable I just got a "No, we keep here, 
private, for now. We don't have a good public API for it".

-- 
/Jacob Carlborg
Oct 03 2013
parent "Dicebot" <public dicebot.lv> writes:
On Thursday, 3 October 2013 at 18:42:30 UTC, Jacob Carlborg wrote:

 On 2013-10-03 16:36, Dicebot wrote:


 That is a bit frustrating part - we (reviewers) kind of want 
 stuff
 cleanly separated between modules for further re-usage but at 
 the same
 time we will make your life hell when reviewing how exactly 
 this stuff
 needs to be incorporated :P


 Every time I asked that some functionality should be place 
 somewhere else because it's generally useable I just got a "No, 
 we keep here, private, for now. We don't have a good public API 
 for it".


I guess I need to re-read the very first thread then :)
Oct 03 2013
prev sibling parent reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 03/10/13 13:31, Dicebot wrote:

 My role as review manager does not matter here. Responsibility of review
manager
 is to collect opinions and information from community, he does not have any
 authority of his own.


It's not about your opinions of the code per se, so much as about your 
experience of what is likely to provide a smooth review process that is less 
likely to end in rejection for trivial rather than major reasons (meaning a 
second round of review and therefore more work for you).


 Remembering std.serialization discussion though, it felt like people are more
in
 favor of merging generic functionality into matching modules instead of keeping
 everything self-contained.

 You can also just do pull requests for smaller parts without any review if they
 are decoupled from std.rational - but be warned that waiting until those are
 reviewed and merged by Phobos devs may take a considerable time ;)


Yes, that's my fear. :-(


 In the end it is your own judgment. There are no strict rules and it is always
 good to see a module with personality behind ;)


I think I will follow Jacob's suggestion to make private all that is not 
_essential_ to have in public, with the scope to pull these out into broader 
Phobos later.  That should allow the essentials of std.rational to go forward 
without disturbing anything else, and then its generically useful stuff can be 
parcelled out later without delaying the module itself.

And of course if there is consensus during the review process that certain 
things _should_ be made public and moved elsewhere, I'll do that.
Oct 03 2013
parent reply "Dicebot" <public dicebot.lv> writes:
On Thursday, 3 October 2013 at 12:16:51 UTC, Joseph Rushton 
Wakeling wrote:

 It's not about your opinions of the code per se, so much as 
 about your experience of what is likely to provide a smooth 
 review process ...


Experience? :D I have not yet finished a single complete review 
process. If you look for experienced advice, ping some guys from 
that list in the bottom : http://wiki.dlang.org/Review_Queue :P
Oct 03 2013
next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 03/10/13 16:38, Dicebot wrote:

 On Thursday, 3 October 2013 at 12:16:51 UTC, Joseph Rushton Wakeling wrote:

 It's not about your opinions of the code per se, so much as about your
 experience of what is likely to provide a smooth review process ...


 Experience? :D I have not yet finished a single complete review process.


You've managed a few, no? :-P

Anyway, here's the state of play: I have two distinct branches that both 
implement std.rational as a new module in Phobos.

https://github.com/WebDrake/phobos/tree/rational implements things as I think 
they should be, with several generic functions/templates parcelled out to 
std.traits and std.numeric.

https://github.com/WebDrake/phobos/tree/rational-standalone implements things
as 
a standalone module with all non-essential functions (or local duplications) 
marked as private.

I would be happy for either or preferably both side-by-side to be subject to 
review now.  I think both are at the point where my asking on the forums is not 
going to get this code the scrutiny it needs.  That said, my concern is that 
there will be some significant changes requested and it may be knocked back
this 
time -- which is why I've tried asking questions on the forums in the first
place.

So really, as review manager, it's your call.  If you'd like me to keep 
following up on my concerns and delay submission, I'll do that, but if you're 
happy to move forward, let's do it. :-)

Thanks & best wishes,

     -- Joe
Oct 04 2013
prev sibling next sibling parent Brad Roberts <braddr puremagic.com> writes:
On 10/4/13 7:16 AM, Joseph Rushton Wakeling wrote:

 On 03/10/13 16:38, Dicebot wrote:

 On Thursday, 3 October 2013 at 12:16:51 UTC, Joseph Rushton Wakeling wrote:

 It's not about your opinions of the code per se, so much as about your
 experience of what is likely to provide a smooth review process ...


 Experience? :D I have not yet finished a single complete review process.


 You've managed a few, no? :-P

 Anyway, here's the state of play: I have two distinct branches that both
implement std.rational as a
 new module in Phobos.

 https://github.com/WebDrake/phobos/tree/rational implements things as I think
they should be, with
 several generic functions/templates parcelled out to std.traits and
std.numeric.

 https://github.com/WebDrake/phobos/tree/rational-standalone implements things
as a standalone module
 with all non-essential functions (or local duplications) marked as private.

 I would be happy for either or preferably both side-by-side to be subject to
review now.  I think
 both are at the point where my asking on the forums is not going to get this
code the scrutiny it
 needs.  That said, my concern is that there will be some significant changes
requested and it may be
 knocked back this time -- which is why I've tried asking questions on the
forums in the first place.

 So really, as review manager, it's your call.  If you'd like me to keep
following up on my concerns
 and delay submission, I'll do that, but if you're happy to move forward, let's
do it. :-)

 Thanks & best wishes,

      -- Joe


Ideally, the unrelated but required non-rational code would be delt with before
the review, then the 
issue is moot.  If you've got important or useful changes to other parts of
phobos, separate them 
and get them delt with.
Oct 04 2013
prev sibling next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 04/10/13 21:00, Brad Roberts wrote:

 Ideally, the unrelated but required non-rational code would be delt with before
 the review, then the issue is moot.  If you've got important or useful changes
 to other parts of phobos, separate them and get them delt with.


I don't mind doing that, but it seemed to me that _all_ of this code deserved 
the level of scrutiny that a formal review would bring, both generic and 
specialized parts.
Oct 04 2013
prev sibling next sibling parent Brad Roberts <braddr puremagic.com> writes:
On 10/4/13 1:39 PM, Joseph Rushton Wakeling wrote:

 On 04/10/13 21:00, Brad Roberts wrote:

 Ideally, the unrelated but required non-rational code would be delt with before
 the review, then the issue is moot.  If you've got important or useful changes
 to other parts of phobos, separate them and get them delt with.


 I don't mind doing that, but it seemed to me that _all_ of this code deserved
the level of scrutiny
 that a formal review would bring, both generic and specialized parts.


That's not an argument against splitting the changes up and getting the
dependencies handled first. 
  It's an argument to do a good job with both sets of changes, which I agree
with.
Oct 04 2013
prev sibling next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 05/10/13 00:40, Brad Roberts wrote:

 That's not an argument against splitting the changes up and getting the
 dependencies handled first.  It's an argument to do a good job with both sets
of
 changes, which I agree with.


Understood, but given that currently the supposedly independent changes are 
_only_ used by std.rational, there is a case for presenting everything in 
context together, because people may have alternative suggestions for how to 
handle these cases which don't rely on new templates.

Yes, I am trying to avoid a situation where I get caught up in weeks of pull 
request feedback before I can even think about submitting std.rational for 
review, but it's not _all_ about saving me time. :-)
Oct 04 2013
prev sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 05/10/13 00:40, Brad Roberts wrote:

 That's not an argument against splitting the changes up and getting the
 dependencies handled first.  It's an argument to do a good job with both sets
of
 changes, which I agree with.


I've submitted a first supporting pull request:
https://github.com/D-Programming-Language/phobos/pull/1616
Oct 05 2013
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 10/2/2013 7:25 AM, Joseph Rushton Wakeling wrote:

 Anyway, I'd really value your feedback at this point.


To add to your burdens :-) please also run a -cov -unittest -main, and
determine 
the unittest coverage. Please shoot for >= 95% coverage. 100% is even better!
Oct 04 2013
parent reply "Joseph Rushton Wakeling" <joseph.wakeling webdrake.net> writes:
On Friday, 4 October 2013 at 18:56:35 UTC, Walter Bright wrote:

 On 10/2/2013 7:25 AM, Joseph Rushton Wakeling wrote:

 Anyway, I'd really value your feedback at this point.


 To add to your burdens :-) please also run a -cov -unittest 
 -main, and determine the unittest coverage. Please shoot for >= 
 95% coverage. 100% is even better!


It is 95% -- I can always see if I can improve that :-)
Oct 04 2013
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 10/4/2013 12:31 PM, Joseph Rushton Wakeling wrote:

 It is 95% -- I can always see if I can improve that :-)
Oct 04 2013
parent reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 04/10/13 21:43, Walter Bright wrote:

 On 10/4/2013 12:31 PM, Joseph Rushton Wakeling wrote:

 It is 95% -- I can always see if I can improve that :-)




It's now 98% :-)

It'd be 99% if these two expressions were each placed on one line instead of 
wrapped, but I figure that would be greedy:
https://github.com/WebDrake/phobos/blob/rational/std/rational.d#L827-L830

Apart from that there's one line that is not an assert(0) that is not covered, 
which I guess just comes down to it being very rare that it should be activated:
https://github.com/WebDrake/phobos/blob/rational/std/rational.d#L593
Oct 04 2013
next sibling parent reply "Brian Schott" <briancschott gmail.com> writes:
On Friday, 4 October 2013 at 21:09:26 UTC, Joseph Rushton 
Wakeling wrote:

 It's now 98% :-)


That's not fair! You're not triggering any bugs in the coverage 
analyzer. :-)
Oct 04 2013
parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 04/10/13 23:12, Brian Schott wrote:

 On Friday, 4 October 2013 at 21:09:26 UTC, Joseph Rushton Wakeling wrote:

 It's now 98% :-)


 That's not fair! You're not triggering any bugs in the coverage analyzer. :-)


Well, unless you count the fact that an assert statement split across 2 lines 
registers as one line covered and one line not, because the error message 
doesn't get printed, which happens because the code works correctly ... :-)

It's got me ever so slightly miffed that if only I was prepared to have an 
uncivilly long line in the code, I'd be on 99% instead.
Oct 04 2013
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 10/4/2013 2:09 PM, Joseph Rushton Wakeling wrote:

 On 04/10/13 21:43, Walter Bright wrote:

 On 10/4/2013 12:31 PM, Joseph Rushton Wakeling wrote:

 It is 95% -- I can always see if I can improve that :-)




 It's now 98% :-)

 It'd be 99% if these two expressions were each placed on one line instead of
 wrapped, but I figure that would be greedy:
 https://github.com/WebDrake/phobos/blob/rational/std/rational.d#L827-L830

 Apart from that there's one line that is not an assert(0) that is not covered,
 which I guess just comes down to it being very rare that it should be
activated:
 https://github.com/WebDrake/phobos/blob/rational/std/rational.d#L593


Methinks you can get the 100% gold!
Oct 04 2013
parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 04/10/13 23:40, Walter Bright wrote:

 Methinks you can get the 100% gold!


I don't think any code containing an assert(0) should be able to get 100%, 
unless it's broken and that assert is actually getting triggered, no?  Well, 
unless you want to revise the coverage analyser to ignore assert(0) and 
assert(false) statements. ;-)

The interesting thing is the lines I highlighted, how those enforce statements 
broken across 2 lines actually come out as one covered line and one not:

     enforce(someCondition,   // gets evaluated/covered
             "This message never gets used so the line is not covered.");

But there is the one single return statement that never gets used.  I'll have
to 
work out exactly what triggers it and make sure there's a unittest for that.
Oct 04 2013
prev sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Wednesday, 2 October 2013 at 14:26:04 UTC, Joseph Rushton 
Wakeling wrote:

 Hello all,

 I thought I'd ask for some guidance here, since this is the 
 first time I'm involved in bringing a new Phobos module to 
 review.


You've gotten some general input, I'll provide mine.


 If I understand right, "review" means that I present a pull 
 request to Phobos, rather than just a standalone piece of code 
 as currently available.  The question is how to handle various 
 bits of std.rational that arguably could or maybe should be 
 implemented elsewhere in Phobos.  Is there a preference to 
 present the module as-is, and have the review process decide 
 what goes where, or is it better to present the pull request 
 including patches against other modules?


People want to be able to review how the code will fit into 
Phobos, having the source incorporated in a branch of Phobos is 
the best way (and makes it easier to generate docs which resemble 
Phobos). A pull request should not be created.



    * Because it doesn't just want to work with built-in integer 
 types, David
      Simcha was forced to define custom versions of various 
 functionality
      found elsewhere in Phobos (or in some cases, perhaps at 
 the time the
      functionality simply was not there).  Some of these have 
 been deleted
      in my recent updates as they are no longer necessary: e.g. 
 today it is
      fine to use std.math.abs, std.traits.isAssignable.  Others 
 remain, and
      the question is how to handle these cases.


If the custom changes can be made to work with the existing 
functions, it can replace those existing functions. E.g. 
std.math.gcd can be updated to accept Rational as long as it 
continues to work on all types.


    * David defines an isRational template, which is currently 
 very simple
      (it just checks that the type has numerator and 
 denominator properties).
      This presumably is fine to keep in std.rational and should 
 not be moved
      elsewhere.


Yes, leave the trait check inside the rational library. We don't 
have an integer... library so std.traits contains a number of 
traits for built in types.


    * Because std.rational doesn't just want to work with 
 built-in integer types
      it can't rely on the existing isIntegral.  Instead, David 
 defined a new
      template, "isIntegerLike", which checks the operations 
 supported by the
      type and whether they work in an integer-like way.  
 Could/should this be
      placed in std.traits?  Better to do this in the initial 
 pull or let the
      decision be part of the review process?


Add to std.traits for below.


    * For similar reasons, CommonType is insufficient and David 
 defined two
      new templates, CommonInteger (which works out an 
 appropriate common
      type for two "integer-like" types) and CommonRational.  
 The former at
      least looks to me like it should be in std.traits.  Again, 
 better to
      do this in the initial pull request, or make it a decision 
 to take at
      review time?


Keep private for now.


    * Several Rational-based overrides for std.math functions 
 are defined:
      floor, ceil and round.  I believe it's normal for such 
 type-specific
      overrides to be in the same module as their type?


Yes, if it is just an override leave it here.


    * Finally, there are two mathematical functions, gcf 
 (greatest common factor)
      and lcm (least common multiple) which again are locally 
 defined because
      Phobos' existing std.math.gcd cannot handle BigInts (let 
 alone any other
      user-defined integer type).  These should presumably be 
 sent over to
      std.math but that relies on isIntegerLike being accepted 
 for std.traits,
      or else some alternative type check being in place.


Fix std.math to work with the new type.

You'll want to get the public interface as polished as possible. 
Another solution would be to remove gcf/lcm from public API, if 
missing those would hold back inclusion then people will tell 
you. Once the module has been accepted then pull requests can be 
done to fix gcd and add lcm and it will not need formal review.
Oct 04 2013