I'm looking forward to reviewing your code within my limited knowledge, however I can't guarantee a quick time frame since Apache GSoC mentor milestones are due next week and I think that could get time consuming.
Thanks for the contribution, Eric On Thu, Jul 18, 2019 at 4:13 PM Heinrich Bohne <heinrich.bo...@gmx.at> wrote: > So, I think the code I have so far is ripe for a pull request, so I > submitted one. I changed the contracts of the epsilon and > max-denominator factory methods, because the old specifications were not > very useful, especially that of the max-denominator method – the only > guarantee you could get from it was that /some/ fraction with a > denominator not greater than the specified maximum would be returned, > without any relation to the double value that should be approximated. > > The simple continued fraction is now created from an exact BigFraction > representation of the double value rather than with floating-point > arithmetic on the double value itself, to preserve maximum precision > (the old epsilon algorithm produces a fraction of 1/3 when passed the > double value obtained from the expression 1.0/3.0 and an epsilon of 5 * > 2^(-58), which is incorrect because the distance between the rational > number 1/3 and its closest double representative is larger than 5 * > 2^(-58); I added a corresponding unit test). > > The methods setLastCoefficient(BigInteger) and removeLastCoefficient() > in the new class SimpleContinuedFraction are unused. I wrote this class > before I implemented the algorithms, and I thought these methods might > be useful in the max-denominator method, but this turned out not to be > the case. However, from a design-perspective, these two methods > complement the functionality of addCoefficient(BigInteger), so I thought > their presence is still tolerable. > > I solved the problem with the maxIterations argument in the epsilon > method by simply not limiting the number of iterations if this argument > is negative. Maybe this parameter was necessary in the old algorithm > which calculated the simple continued fraction with floating-point > arithmetic, to prevent an infinite loop or something. > > On 7/17/19 10:20 AM, Heinrich Bohne wrote: > > It just occured to me that you might have misunderstood my sentence: > > > >> I am even more confused by your suggestion seeing as it was > >> you who banned BigInteger from Fraction.addSub(Fraction, boolean) in > >> https://issues.apache.org/jira/browse/NUMBERS-79 , which, even though > >> you were not aware of it at that time, did not limit the method's > >> functionality in any way whatsoever > > > > The "which" was referring to your removal of BigInteger, not to the use > > of BigInteger prior to your edits, so what I meant to say was, by > > removing BigInteger, you did not limit the method's functionality. > > > > > > On 7/17/19 10:10 AM, Heinrich Bohne wrote: > >>> The reason it was done was because Knuth proved > >>> (as in mathematical proof) that a long is insufficient for certain > >>> fraction > >>> multiplications where both numerator and denominator are large ints; 65 > >>> rather than 64 bits are necessary and a long will not suffice. > >> > >> You seem to have missed my comment in ticket > >> https://issues.apache.org/jira/browse/NUMBERS-79 , which you created – > I > >> don't have the book by D. Knuth, but I can only assume that the section > >> referenced by the code talks about unsigned integers, because by the > >> logic in the comment I left in the JIRA ticket, long values are > >> **always** sufficient in Fraction.addSub(Fraction, boolean). > >> > >> But this is beside the point, I only mentioned it because I didn't > >> understand why you suggested to remove the BigFraction class, and > >> actually, I still don't, as the class BigFraction provides functionality > >> that Fraction cannot have, both with and without my suggested > >> alterations. > >> > >> > >> On 7/17/19 2:29 AM, Eric Barnhill wrote: > >>> On Tue, Jul 16, 2019 at 2:41 PM Heinrich Bohne<heinrich.bo...@gmx.at> > >>> wrote: > >>> > >>>>> Do you think we really even need a BigFraction class at all in the > >>>> context > >>>>> of these upgrades? Or should one of the Fraction factory methods just > >>>> take > >>>>> BigInteger argumentsm and all fractions use the lazy dynamic > >>>>> method of > >>>>> calculation you are proposing? > >>>> I don't quite understand what you mean by this. The BigInteger class > >>>> provides flexibility and the ability to store and operate on > >>>> (practically) unlimited values, which Fraction does not have. The > >>>> Fraction class, on the other hand, is faster and more memory > >>>> efficient, > >>>> due to its use of primitive values, which is an advantage over > >>>> BigFraction. > >>> That's fine. > >>> > >>> > >>>> I am even more confused by your suggestion seeing as it was > >>>> you who banned BigInteger from Fraction.addSub(Fraction, boolean) in > >>>> https://issues.apache.org/jira/browse/NUMBERS-79 , which, even > though > >>>> you were not aware of it at that time, did not limit the method's > >>>> functionality in any way whatsoever (the use of int rather than long > >>>> did, however, but this is now fixed). > >>>> > >>> I don't know what you mean by "functionality" but constructing a > >>> BigInteger > >>> for every fraction multiplication uses up more memory and operations > >>> than > >>> necessary and scales poorly. BigIntegers are not fast. > >>> > >>> However, I understand why the previous coders incorporated a > >>> BigInteger and > >>> I'm not sure that you do. The reason it was done was because Knuth > >>> proved > >>> (as in mathematical proof) that a long is insufficient for certain > >>> fraction > >>> multiplications where both numerator and denominator are large ints; 65 > >>> rather than 64 bits are necessary and a long will not suffice. For me, > >>> these cases are so extreme and likely so rare that we might as well let > >>> them fail, report to the user that these cases need to be handled with > >>> BigFraction and leave it there. It could easily be handled in a try > >>> catch > >>> block and such a block would be high performance. > >>> > >>> That was the judgment I made and it is open to interpretation, provided > >>> such interpretation agrees with Knuth's proof. We are entitled to our > >>> own > >>> opinions but not our own facts. > >>> > >>> Anyway I think your approximation schemes sound good and implement them > >>> however you see fit. > >>> > >>> Eric > >>> > >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > >> For additional commands, e-mail: dev-h...@commons.apache.org > >> > >> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
