from:"Heinrich Bohne"

Re: [numbers-core] Special cases in greatest common divisor methods

2019-07-20 Thread Heinrich Bohne


This separates the result into three:

- representable signed integer
- representable unsigned integer (your new Integer.MIN_VALUE case)
- unknown result (exception)


Maybe I misunderstood you, but with my suggested changes, the method
would never throw an exception, because the gcd of two (signed) ints can
never be greater than 2^31 (just like with the methods Math.abs(int) and
Math.abs(long)), and the case of 2^31 is the /only/ case where the
method currently throws an exception.


If you change the current method, documented to never return a negative value, 
dependent downstream code may break. Since this is unreleased code the 
downstream code can be assumed to be just that in [numbers] so this may not be 
a major issue.


I've already looked through the usages of the two gcd methods in
[numbers], and there are very few, and none of them rely on the method
throwing an exception if the gcd is the max value of the corresponding
data type (although one usage case could be simplified, which is the
example in the Fraction(int, int) constructor I mentioned).

As you yourself said, separate methods like gcdUnsigned, while
potentially useful, would provide a solution to a completely different
problem than the one I described, which is that the /arguments/ should
be treated as unsigned integers, not the result.


On 7/20/19 2:30 PM, Alex Herbert wrote:



On 20 Jul 2019, at 10:53, Heinrich Bohne  wrote:

I have suggestion regarding what to do with the special cases
Integer.MIN_VALUE and Long.MIN_VALUE in the methods from ArithmeticUtils
that calculate the greatest common divisor of two ints or longs.
Currently, these methods throw an ArithmeticException if the GCD of the
arguments is 2^31 or 2^63, respectively. However, what if, instead,
these methods simply returned -2^31 and -2^63 in these special cases? I
think this would have several advantages:

  - By returning -2^31 and -2^63, the methods will /always/ return a
unique "representation" of the GCD, even if this representation may,
under one predictable circumstance, not be the GCD itself but its
additive inverse. An exception, on the other hand, implies that the GCD
cannot be represented and is therefore less useful.
  - Throwing an exception is more expensive than returning -2^31 or -2^63.
  - If the distinction between the GCD and its additive inverse matters
to the caller, the caller would have to handle the case of
Integer.MIN_VALUE or Long.MIN_VALUE separately in any case, whether the
method throws an exception or returns -2^31 or -2^63. There may,
however, be cases, where this distinction does not matter, for example,
if the caller just wants to check if two numbers are coprime or not (in
fact, there already is such a case, namely in the constructor
Fraction(int, int) from the fraction module). An exception would then
force the caller to handle the case separately, whereas this would not
be necessary if the method returned -2^31 or -2^63.
  - The practice of returning Integer.MIN_VALUE and Long.MIN_VALUE in
place of 2^31 and 2^63 is precedented in the Java library itself:
Math.abs(int) and Math.abs(long) return the respective min values if the
argument is that min value, so a similar design in the gcd methods would
be more consistent with the Java API than throwing an exception.

This separates the result into three:

- representable signed integer
- representable unsigned integer (your new Integer.MIN_VALUE case)
- unknown result (exception)

If you change the current method, documented to never return a negative value, 
dependent downstream code may break. Since this is unreleased code the 
downstream code can be assumed to be just that in [numbers] so this may not be 
a major issue.

I can see the usefulness in this approach if the input int/long are treated as 
unsigned integers. If treated as signed integers then to get the 
Integer.MIN_VALUE case for the GCD would require at least one argument being 
Integer.MIN_VALUE. This is an extreme edge case.

Is there merit in a duplicate method to handle unsigned integers:

ArithmeticUtils.gcdUnsigned(int, int)
ArithmeticUtils.gcdUnsigned(long, long)

However if this follows the JDK 8 unsigned method convention 
(Integer.toUnsignedString, Integer.divideUnsigned, etc) the input would be 
assumed to already be unsigned bits. So if you are working with signed integers 
you would have to call this using Math.abs() to convert twos compliment 
negative values to unsigned:

int v1 = Integer.MIN_VALUE;
int v2 = Integer.MIN_VALUE;
// Greatest common divisor as unsigned 32-bit int
int gcd = ArithmeticUtils.gcdUnsigned(Math.abs(v1), Math.abs(v2));

This is more work than updating the current method, documented to only work in 
the range up to 2^31-1, and I don’t have use cases to determine if an extra 
method is worth it.

So:

1. Make your changes and fix any dependent code in [numbers]
2. Add new unsigned methods and change any dependent code in [numbers] that 
care about this edge case result

[numbers-core] Special cases in greatest common divisor methods

2019-07-20 Thread Heinrich Bohne


I have suggestion regarding what to do with the special cases
Integer.MIN_VALUE and Long.MIN_VALUE in the methods from ArithmeticUtils
that calculate the greatest common divisor of two ints or longs.
Currently, these methods throw an ArithmeticException if the GCD of the
arguments is 2^31 or 2^63, respectively. However, what if, instead,
these methods simply returned -2^31 and -2^63 in these special cases? I
think this would have several advantages:

 - By returning -2^31 and -2^63, the methods will /always/ return a
unique "representation" of the GCD, even if this representation may,
under one predictable circumstance, not be the GCD itself but its
additive inverse. An exception, on the other hand, implies that the GCD
cannot be represented and is therefore less useful.
 - Throwing an exception is more expensive than returning -2^31 or -2^63.
 - If the distinction between the GCD and its additive inverse matters
to the caller, the caller would have to handle the case of
Integer.MIN_VALUE or Long.MIN_VALUE separately in any case, whether the
method throws an exception or returns -2^31 or -2^63. There may,
however, be cases, where this distinction does not matter, for example,
if the caller just wants to check if two numbers are coprime or not (in
fact, there already is such a case, namely in the constructor
Fraction(int, int) from the fraction module). An exception would then
force the caller to handle the case separately, whereas this would not
be necessary if the method returned -2^31 or -2^63.
 - The practice of returning Integer.MIN_VALUE and Long.MIN_VALUE in
place of 2^31 and 2^63 is precedented in the Java library itself:
Math.abs(int) and Math.abs(long) return the respective min values if the
argument is that min value, so a similar design in the gcd methods would
be more consistent with the Java API than throwing an exception.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

2019-07-19 Thread Heinrich Bohne


Sure, no problem, thanks for letting me know!

On 7/19/19 6:22 PM, Eric Barnhill wrote:

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 
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
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 fra

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

2019-07-18 Thread Heinrich Bohne


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
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 l

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

2019-07-17 Thread Heinrich Bohne


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
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

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

2019-07-17 Thread Heinrich Bohne


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
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

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

2019-07-16 Thread Heinrich Bohne


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. 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).

What I meant by lazy calculation was that the helper method does not
return the complete simple continued fraction expansion of the given
double value at once, because not all coefficients are needed to
approximate the number if the denominator limit is exceeded before the
last convergent is calculated (or if the convergent is close enough to
the actual value, in epsilon mode). Rather, the coefficients can be
queried one-by-one through an iterator, so that they are only calculated
when they are really needed. I don't see how this could replace or help
with any other functionality of the two fraction classes, though. It's
not like any state associated with the fraction instance itself would be
lazily evaluated. The calculation of the continued fraction expansion is
only relevant for the creation of the instance.

In fact, I have already written the helper class including Javadoc and
unit tests. It turns out that I implemented more methods than I actually
need, but the unneeded methods don't hurt, so I've left them in the class.

I am also finished with implementing the algorithm for approximating a
double number with a denominator limit. The algorithm itself is not very
complicated, but what I found all the more difficult was explaining
_why_ it works without the code comments becoming a thesis, but still in
a way that a person who is not an expert with continued fractions will
understand (which includes me, so I should hopefully be able to tell …).
I'm using some sections from the book Continued Fractions by Aleksandr
Khinchin thatare available in the preview on Google Books as a
reference, in addition to a question from Math Stackexchange.

Now I'm going to delete the unit tests asserting that the factory method
throws an exception if the double value is outside the int range >D

By the way, I think I'll also add a method that allows just an epsilon
to be specified without a parameter maxIterations, because currently,
the only public method for an epsilon-approximation also requires a
maximum number of iterations to be specified, which I think is
ridiculous, not least because the number of iterations seems to me to be
an implementation detail which the user is not necessarily interested
in, and there is no reason not to guarantee that the method will return
an appropriate approximation.


On 7/16/19 6:52 PM, Eric Barnhill wrote:

Sorry for the delay, I was on vacation.

On Fri, Jul 5, 2019 at 2:09 PM Heinrich Bohne  wrote:


Hello!

I think a re-design of the factory method BigFraction.from(double,
double, int, int) is in order, because I see several problems with it:

First, having a separate fraction class intended to overcome the
limitations of the int range with a factory method (formerly a
constructor) for approximating double values that can only produce
denominators within the int range because it has been copy-pasted from
Fraction (where this code is still a constructor) seems a bit like a
joke. I think it would be more useful to have this method accept a
BigInteger as an upper bound for the denominator instead of an int.


Quite right! I wanted to look this up before replying. It absolutely makes
sense to use a BigInteger there.



Second, the method only calculates the convergents of the corresponding
continued fraction, but never its semi-convergents, so it doesn't
necessarily produce the best rational approximation of the double number
within the given bounds. For example, the test method
BigFractionTest.testDigitLimitConstructor() asserts that the method
calculates 3/5 as an approximation of 0.6152 with the upper bound for
the denominator set to 9, but 5/8 = 0.625 is closer to 0.6152 than 3/5 =
0.6. Since the method is already using continued fractions to
approximate fractional numbers, I think it would be a pity if it didn't
take advantage of them for all that they're worth.


Wow. That is indeed problematic, nice catch.



Finally, the documentation of the method rightfully acknowledges the
latter's confusing design, with the metho

[numbers-fraction] Double approximation constructor/factory method overhaul

2019-07-05 Thread Heinrich Bohne


Hello!

I think a re-design of the factory method BigFraction.from(double,
double, int, int) is in order, because I see several problems with it:

First, having a separate fraction class intended to overcome the
limitations of the int range with a factory method (formerly a
constructor) for approximating double values that can only produce
denominators within the int range because it has been copy-pasted from
Fraction (where this code is still a constructor) seems a bit like a
joke. I think it would be more useful to have this method accept a
BigInteger as an upper bound for the denominator instead of an int.

Second, the method only calculates the convergents of the corresponding
continued fraction, but never its semi-convergents, so it doesn't
necessarily produce the best rational approximation of the double number
within the given bounds. For example, the test method
BigFractionTest.testDigitLimitConstructor() asserts that the method
calculates 3/5 as an approximation of 0.6152 with the upper bound for
the denominator set to 9, but 5/8 = 0.625 is closer to 0.6152 than 3/5 =
0.6. Since the method is already using continued fractions to
approximate fractional numbers, I think it would be a pity if it didn't
take advantage of them for all that they're worth.

Finally, the documentation of the method rightfully acknowledges the
latter's confusing design, with the method's general behavior being
dependent on some of its arguments and the validity of these arguments
also being dependent on each other. However, a better way to solve this
problem than to simply hide the design from the public would be to
improve it, e.g. by extracting the functionality that is common to both
the "maxDenominator mode" and the epsilon mode (which is the calculation
of the continued fraction), and separating the differences in the
functionality of the two modes into distinct methods that call the
common functionality.

My suggestion for the third point above would be to create a separate
class (not necessarily public) that provides an interface for
calculating simple continued fractions and their convergents (I see that
there's an abstract class ContinuedFraction, but I don't think it will
be useful, because all the methods only return double values, and the
class also requires that all coefficients can be explicitly calculated
based on their index). The class would ideally be able to calculate the
continued fraction dynamically/lazily, because only a limited number of
coefficients are needed to approximate a fractional number within given
bounds. What I think could be useful is if the class stores a list of
the coefficients internally in addition to the current and previous
convergent (two consecutive convergents are needed to calculate the next
one recursively based on the next coefficient), and has methods like
addCoefficient(BigInteger) and removeLastCoefficient() for building a
continued fraction, and also a static method like
coefficientsOf(BigFraction) that returns an Iterator that
computes the coefficients only as they are queried through the iterator,
so that they can then be passed to addCoefficient(BigInteger).

The maxDenominator factory method could then just iterate over the
coefficients of the continued fraction representation of the passed
double and build the continued fraction from them until the denominator
of the current convergent exceeds the upper bound, and the epsilon
method could iterate over the coefficients of both the lower and upper
bound's continued fraction representation until the coefficients start
to differ, at which point it can build the continued fraction of the
close enough approximation from all coefficients at once (this would
also prevent any loss of precision when repeatedly performing arithmetic
operations with floating-point values).

Furthermore, this code could not only be used by the approximation
factory methods in BigFraction, but also by those in Fraction, possibly
adjusted so that not only the denominator must be within a given bound,
but also the numerator needs to be within the int range.

Any opinions or objections?

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: False coverage decrease accusations by Coveralls

2019-07-03 Thread Heinrich Bohne

It is very strange indeed, because the last time this happened, the
reported change in coverage percentage was also wrong, so I initially
assumed that the -0.03% report for this pull request time was false too.
Actually, I'm still not entirely convinced that it isn't false, because
in the summary above, it says "40 of 42 new or added lines in 1 file
covered.", but in the menu "SOURCE CHANGED", it only says that 21
relevant lines have been added to "BigFraction", 19 of which are
covered. Anyway, thanks for taking the time to look into this.

On 7/3/19 1:24 PM, Alex Herbert wrote:

On 03/07/2019 12:00, Heinrich Bohne wrote:

I think we are talking about two completely different issues here. I am
aware that 2 of my newly introduced lines (the IllegalArgumentException
cases you mentioned) are not covered. These are argument validations
inside private methods, so they should never be reached, as you
correctly assumed.

What I meant, however, is, for example, the method
BigFraction.toString(). According to Coveralls, my pull request caused
several lines inside this method that were previously covered to be
uncovered. But both https://coveralls.io/builds/24307694 (which seems to
be the version of the master branch upon which the comparison in the
report for my pull request is based) and
https://coveralls.io/builds/24338122 (which is the current version of
master) already list these lines as uncovered, so my pull request did
uncover these lines in BigFraction.toString(), contrary to what the
Coverall report says.

Here are the links that directly point to the relevant lines in the
report for BigFraction:

https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273

(my pull request)

https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128

(the master version against which my pull request was compared)

https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127

(the current master version)

OK I see. You are not disputing the overall coverage but the report on
what has been uncovered. So Coveralls cannot link back to the previous
coverage report where they were also not covered. It is a bug to raise
with Coveralls; or not worry about it.

On 7/3/19 12:32 PM, Alex Herbert wrote:

On 03/07/2019 10:35, Heinrich Bohne wrote:

But the detailed report you linked to is exactly where I got the
information about what existing lines have (purportedly) been
uncovered
from. It's true that the master branch changed in the meantime, but
those commits only concerned formatting and changing the field
serialVersionUID in BigFraction and Fraction. I don't understand
exactly
what this variable is for, but I doubt that it has something to do
with
the respective lines now being uncovered. In fact, the corresponding
build https://coveralls.io/builds/24338122 lists the two files
BigFraction.java and Fraction.java under the menu "COVERAGE CHANGED",
but it doesn't actually report any change in coverage, so maybe
this has
something to do with the bug.

Your changes made:

commons-numbers-fraction: 88.31% to 88.41%

Overall: 2673 of 2836 (94.252%) to 2692 of 2857 (94.22%)

This issue is that although you increased coverage in
commons-numbers-fraction because the increase is lower than the
average across all modules you get an overall lowering of total
coverage.

Your new changes have 40/42 (95.24%) coverage. This is below the
previous overall average so the score is lower.

The two missed lines are in your new functions toFloatingPointBits and
roundAndRightShift which have an uncovered IllegalArgumentException
edge case. Are these ever possible to hit? If not then you should put
in a message for the exception, for example "Internal error: Please
file a bug report".

This should make it clear that the exception is not meant to be
possible but the assertion it makes is a requirement for the rest of
the function to work.

On 7/3/19 11:19 AM, Alex Herbert wrote:

On 3 Jul 2019, at 09:38, Heinrich Bohne
wrote:

So this is the second time this happens to me. I've submitted a pull
request ( https://github.com/apache/commons-numbers/pull/63 ),
and the
Coveralls reports says that several existing lines have been
uncovered,
which is a lie, because the lines purportedly "uncovered" were
already
not covered in the master branch (specifically the method
BigFraction.toString(), and, in the class Fraction, some lines in
addSub(Fraction, boolean), toString(), zero(), one() and
parse(String)).
Something should probably be done about this, but I don't know the
right
place where to report this.

You can click on the Coveralls badge on Github to get the detailed
report of what changed:

https

Re: False coverage decrease accusations by Coveralls

2019-07-03 Thread Heinrich Bohne

so my pull request did
uncover these lines in BigFraction.toString(), contrary to what the
Coverall report says.

Excuse me, of course it should be "my pull request did *not*
uncover these lines"

On 7/3/19 1:00 PM, Heinrich Bohne wrote:

Here are the links that directly point to the relevant lines in the
report for BigFraction:

https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273

(my pull request)

https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128

(the master version against which my pull request was compared)

https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127

(the current master version)

On 7/3/19 12:32 PM, Alex Herbert wrote:

On 03/07/2019 10:35, Heinrich Bohne wrote:

But the detailed report you linked to is exactly where I got the
information about what existing lines have (purportedly) been uncovered
from. It's true that the master branch changed in the meantime, but
those commits only concerned formatting and changing the field
serialVersionUID in BigFraction and Fraction. I don't understand
exactly
what this variable is for, but I doubt that it has something to do with
the respective lines now being uncovered. In fact, the corresponding
build https://coveralls.io/builds/24338122 lists the two files
BigFraction.java and Fraction.java under the menu "COVERAGE CHANGED",
but it doesn't actually report any change in coverage, so maybe this
has
something to do with the bug.

Your changes made:

commons-numbers-fraction: 88.31% to 88.41%

Overall: 2673 of 2836 (94.252%) to 2692 of 2857 (94.22%)

This issue is that although you increased coverage in
commons-numbers-fraction because the increase is lower than the
average across all modules you get an overall lowering of total
coverage.

Your new changes have 40/42 (95.24%) coverage. This is below the
previous overall average so the score is lower.

This should make it clear that the exception is not meant to be
possible but the assertion it makes is a requirement for the rest of
the function to work.

On 7/3/19 11:19 AM, Alex Herbert wrote:

On 3 Jul 2019, at 09:38, Heinrich Bohne
wrote:

So this is the second time this happens to me. I've submitted a pull
request ( https://github.com/apache/commons-numbers/pull/63 ), and
the
Coveralls reports says that several existing lines have been
uncovered,
which is a lie, because the lines purportedly "uncovered" were
already
not covered in the master branch (specifically the method
BigFraction.toString(), and, in the class Fraction, some lines in
addSub(Fraction, boolean), toString(), zero(), one() and
parse(String)).
Something should probably be done about this, but I don't know the
right
place where to report this.

You can click on the Coveralls badge on Github to get the detailed
report of what changed:

https://coveralls.io/builds/24338717
<https://coveralls.io/builds/24338717>

This requires a bit of digesting. It seems to have been confused by
the removal of lots of lines and addition of lines to the same file.
It thinks you have 19 new lines covered and 2 extra lines missed in
BigFraction.

Did you rebase your change against master? Perhaps the reference
master it is comparing to is slightly different.

If you care then you could run locally with Jacoco.

What my inspection does show is that a lot of edge cases are not
being covered by tests (divide by zero, addition of zero, etc). This
is more important to fix.

Re: False coverage decrease accusations by Coveralls

2019-07-03 Thread Heinrich Bohne

Here are the links that directly point to the relevant lines in the
report for BigFraction:

https://coveralls.io/builds/24338717/source?filename=commons-numbers-fraction%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcommons%2Fnumbers%2Ffraction%2FBigFraction.java#L1273
(my pull request)

https://coveralls.io/builds/24307694/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1128
(the master version against which my pull request was compared)

https://coveralls.io/builds/24338122/source?filename=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/BigFraction.java#L1127
(the current master version)

On 7/3/19 12:32 PM, Alex Herbert wrote:

On 03/07/2019 10:35, Heinrich Bohne wrote:

But the detailed report you linked to is exactly where I got the
information about what existing lines have (purportedly) been uncovered
from. It's true that the master branch changed in the meantime, but
those commits only concerned formatting and changing the field
serialVersionUID in BigFraction and Fraction. I don't understand exactly
what this variable is for, but I doubt that it has something to do with
the respective lines now being uncovered. In fact, the corresponding
build https://coveralls.io/builds/24338122 lists the two files
BigFraction.java and Fraction.java under the menu "COVERAGE CHANGED",
but it doesn't actually report any change in coverage, so maybe this has
something to do with the bug.

Your changes made:

commons-numbers-fraction: 88.31% to 88.41%

Overall: 2673 of 2836 (94.252%) to 2692 of 2857 (94.22%)

This issue is that although you increased coverage in
commons-numbers-fraction because the increase is lower than the
average across all modules you get an overall lowering of total coverage.

Your new changes have 40/42 (95.24%) coverage. This is below the
previous overall average so the score is lower.

This should make it clear that the exception is not meant to be
possible but the assertion it makes is a requirement for the rest of
the function to work.

On 7/3/19 11:19 AM, Alex Herbert wrote:

On 3 Jul 2019, at 09:38, Heinrich Bohne wrote:

So this is the second time this happens to me. I've submitted a pull
request ( https://github.com/apache/commons-numbers/pull/63 ), and the
Coveralls reports says that several existing lines have been
uncovered,
which is a lie, because the lines purportedly "uncovered" were already
not covered in the master branch (specifically the method
BigFraction.toString(), and, in the class Fraction, some lines in
addSub(Fraction, boolean), toString(), zero(), one() and
parse(String)).
Something should probably be done about this, but I don't know the
right
place where to report this.

You can click on the Coveralls badge on Github to get the detailed
report of what changed:

https://coveralls.io/builds/24338717
<https://coveralls.io/builds/24338717>

Did you rebase your change against master? Perhaps the reference
master it is comparing to is slightly different.

If you care then you could run locally with Jacoco.

What my inspection does show is that a lot of edge cases are not
being covered by tests (divide by zero, addition of zero, etc). This
is more important to fix.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

-
To unsubscribe, e-mail

Re: False coverage decrease accusations by Coveralls

2019-07-03 Thread Heinrich Bohne


But the detailed report you linked to is exactly where I got the
information about what existing lines have (purportedly) been uncovered
from. It's true that the master branch changed in the meantime, but
those commits only concerned formatting and changing the field
serialVersionUID in BigFraction and Fraction. I don't understand exactly
what this variable is for, but I doubt that it has something to do with
the respective lines now being uncovered. In fact, the corresponding
build https://coveralls.io/builds/24338122 lists the two files
BigFraction.java and Fraction.java under the menu "COVERAGE CHANGED",
but it doesn't actually report any change in coverage, so maybe this has
something to do with the bug.

On 7/3/19 11:19 AM, Alex Herbert wrote:



On 3 Jul 2019, at 09:38, Heinrich Bohne  wrote:

So this is the second time this happens to me. I've submitted a pull
request ( https://github.com/apache/commons-numbers/pull/63 ), and the
Coveralls reports says that several existing lines have been uncovered,
which is a lie, because the lines purportedly "uncovered" were already
not covered in the master branch (specifically the method
BigFraction.toString(), and, in the class Fraction, some lines in
addSub(Fraction, boolean), toString(), zero(), one() and parse(String)).
Something should probably be done about this, but I don't know the right
place where to report this.


You can click on the Coveralls badge on Github to get the detailed report of 
what changed:

https://coveralls.io/builds/24338717 <https://coveralls.io/builds/24338717>

This requires a bit of digesting. It seems to have been confused by the removal 
of lots of lines and addition of lines to the same file. It thinks you have  19 
new lines covered and 2 extra lines missed in BigFraction.

Did you rebase your change against master? Perhaps the reference master it is 
comparing to is slightly different.

If you care then you could run locally with Jacoco.

What my inspection does show is that a lot of edge cases are not being covered 
by tests (divide by zero, addition of zero, etc). This is more important to fix.


-
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

False coverage decrease accusations by Coveralls

2019-07-03 Thread Heinrich Bohne


So this is the second time this happens to me. I've submitted a pull
request ( https://github.com/apache/commons-numbers/pull/63 ), and the
Coveralls reports says that several existing lines have been uncovered,
which is a lie, because the lines purportedly "uncovered" were already
not covered in the master branch (specifically the method
BigFraction.toString(), and, in the class Fraction, some lines in
addSub(Fraction, boolean), toString(), zero(), one() and parse(String)).
Something should probably be done about this, but I don't know the right
place where to report this.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

2019-07-02 Thread Heinrich Bohne


Agreed, but it's not clear to me that it must be part of the public API.


Yes, I admit that the public API doesn't really have anything to do with
this, I realize that now. If my last email suggested otherwise, this was
unintentional.


On 7/1/19 11:52 PM, Gilles Sadowski wrote:

Hello.

Le lun. 1 juil. 2019 à 20:23, Heinrich Bohne  a écrit :

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?

Hm, maybe you're right and specifying this (as well as where the sign is
stored) in the public API is not that important, as long as the
combination of returned numerator and denominator is valid. The only
benefit would have been that the user would have the certainty that the
returned values are already reduced to lowest terms, so that, in case
this matters to the user, they don't need to reduce the values
themselves. But whether this would outweigh the reduced flexibility of
the API is something that probably cannot be answered without the
ability to predict the future.

However, what *is* crucial is specifying these things in the
documentation of the private fields that hold the numerator and
denominator, because a lot of the class' implementation relies on this.

Sure.
The code doc should indicate all current assumptions.
By not leaking them into the public API, we keep the flexibility until
usage requires to go one way or another.


In fact, I noticed that BigFraction.hashCode() was broken before the
resolution of NUMBERS-119 and NUMBERS-125, because, while
BigFraction.equals(Object) reduced the numerators and denominators
before comparing them, hashCode() didn't.

Specifying the reduction to lowest terms of the fields numerator and
denominator is even more important in Fraction than in BigFraction,
because the magnitude of the values stored in these fields has an impact
on whether arithmetic operations overflow or don't.

Agreed, but it's not clear to me that it must be part of the public API.

Gilles


On 7/1/19 7:03 PM, Gilles Sadowski wrote:

Hello.

Le lun. 1 juil. 2019 à 12:35, Heinrich Bohne  a écrit :

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?

Yes, it would, but I can't envision a scenario where this would be
detrimental. Of course, I may be missing something.

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?


But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

What I meant is that it would be an implementation detail how the
numerator and denominator are stored. What the methods getNumerator()
and getDenominator() return should, of course, be specified
appropriately (for example, something like "returns the numerator of
this fraction's representation in lowest terms").

It just occurred to me that the question of where something should be
documented can also be applied to the sign being held by the numerator
rather than the denominator in case of negative fractions (which, if I'm
not mistaken, is currently only specified in
Fraction.getReducedFraction(int, int), a method that is targeted for
deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).

Good catch.
It's a similar issue: Is knowing where the sign is stored useful to users or
can it be considered an implementation detail (leaving more freedom to
change it)?
Wouldn't a method "isNegative()" be a better alternative?

Regards,
Gilles



On 7/1/19 9:32 AM, Gilles Sadowski wrote:

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :

I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

2019-07-01 Thread Heinrich Bohne


Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?


Hm, maybe you're right and specifying this (as well as where the sign is
stored) in the public API is not that important, as long as the
combination of returned numerator and denominator is valid. The only
benefit would have been that the user would have the certainty that the
returned values are already reduced to lowest terms, so that, in case
this matters to the user, they don't need to reduce the values
themselves. But whether this would outweigh the reduced flexibility of
the API is something that probably cannot be answered without the
ability to predict the future.

However, what *is* crucial is specifying these things in the
documentation of the private fields that hold the numerator and
denominator, because a lot of the class' implementation relies on this.
In fact, I noticed that BigFraction.hashCode() was broken before the
resolution of NUMBERS-119 and NUMBERS-125, because, while
BigFraction.equals(Object) reduced the numerators and denominators
before comparing them, hashCode() didn't.

Specifying the reduction to lowest terms of the fields numerator and
denominator is even more important in Fraction than in BigFraction,
because the magnitude of the values stored in these fields has an impact
on whether arithmetic operations overflow or don't.

On 7/1/19 7:03 PM, Gilles Sadowski wrote:

Hello.

Le lun. 1 juil. 2019 à 12:35, Heinrich Bohne  a écrit :

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?

Yes, it would, but I can't envision a scenario where this would be
detrimental. Of course, I may be missing something.

Is there a case where *not* knowing whether the fraction is reduced or
not is detrimental?


But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

What I meant is that it would be an implementation detail how the
numerator and denominator are stored. What the methods getNumerator()
and getDenominator() return should, of course, be specified
appropriately (for example, something like "returns the numerator of
this fraction's representation in lowest terms").

It just occurred to me that the question of where something should be
documented can also be applied to the sign being held by the numerator
rather than the denominator in case of negative fractions (which, if I'm
not mistaken, is currently only specified in
Fraction.getReducedFraction(int, int), a method that is targeted for
deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).

Good catch.
It's a similar issue: Is knowing where the sign is stored useful to users or
can it be considered an implementation detail (leaving more freedom to
change it)?
Wouldn't a method "isNegative()" be a better alternative?

Regards,
Gilles




On 7/1/19 9:32 AM, Gilles Sadowski wrote:

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :

I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods as well.

The doc should be fixed.
But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

Regards,
Gilles


Also, fractions returned from arithmetic operations are thereby also
implicitly specified as being reduced to lowest terms, rendering
mentions of this fact in these methods' documentation obsolete.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional comma

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

2019-07-01 Thread Heinrich Bohne


Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Yes, it would, but I can't envision a scenario where this would be
detrimental. Of course, I may be missing something.


But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.


What I meant is that it would be an implementation detail how the
numerator and denominator are stored. What the methods getNumerator()
and getDenominator() return should, of course, be specified
appropriately (for example, something like "returns the numerator of
this fraction's representation in lowest terms").

It just occurred to me that the question of where something should be
documented can also be applied to the sign being held by the numerator
rather than the denominator in case of negative fractions (which, if I'm
not mistaken, is currently only specified in
Fraction.getReducedFraction(int, int), a method that is targeted for
deletion in https://issues.apache.org/jira/browse/NUMBERS-125 ).


On 7/1/19 9:32 AM, Gilles Sadowski wrote:

Hi.

Le lun. 1 juil. 2019 à 03:52, Heinrich Bohne  a écrit :

I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods?

Couldn't it entail a loss of opportunity to allow "non-reduced" fractions
for efficiency reason?


Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods as well.

The doc should be fixed.
But I don't follow; if it's an implementation detail, it should not appear
anywhere in the doc, and users should not rely on it.

Regards,
Gilles


Also, fractions returned from arithmetic operations are thereby also
implicitly specified as being reduced to lowest terms, rendering
mentions of this fact in these methods' documentation obsolete.

-
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

[numbers-fraction] Documentation of fractions' reduction to lowest terms

2019-06-30 Thread Heinrich Bohne


I've recently been wondering about the following:

With the resolution of NUMBERS-119
(https://issues.apache.org/jira/browse/NUMBERS-119), all constructors in
Fraction and BigFraction reduce the created fraction to lowest terms (in
the constructor Fraction(double, double, int, int), this is not obvious,
but because the convergents of a simple continued fraction are
automatically reduced to lowest terms when calculated with the recursive
formula used in the constructor, the fraction returned thereof is also
reduced to lowest terms).

This rises the following question: Would it not be better if this
reduction to lowest terms were made part of the contract of the two
fields storing the numerator and denominator (i.e. included in their
documentation), and, as a consequence, of the corresponding public
accessor methods? Then the fact that the fraction is reduced to lowest
terms when created from a constructor or a public factory method would
become an implementation detail irrelevant to the caller of the
method/constructor, which could also prevent confusion, seeing as the
numerator-denominator factory methods are the only ones that explicitly
state that the fraction is reduced to lowest terms, even though this is
true for all other factory methods as well.

Also, fractions returned from arithmetic operations are thereby also
implicitly specified as being reduced to lowest terms, rendering
mentions of this fact in these methods' documentation obsolete.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

2019-06-21 Thread Heinrich Bohne


I've created a pull request:
https://github.com/apache/commons-numbers/pull/55

This pull request exterminates most the code duplication between the two
classes. There is still some duplication left, most notably in the
method testDigitLimitConstructor(), but I've found out that the
Fraction(double, double, int, int) constructor (and the corresponding
BigFraction constructor, which seems to have been copy-pasted from
Fraction) doesn't work properly anyway, because it doesn't always
generate the closest possible approximation within the given bounds.

For example, 5/8 = 0.625 is closer to 0.6152 than 3/5 = 0.6, but
apparently, the constructor returns 3/5 when 9 is passed as
maxDenominator, and since the test asserts this, the test is useless.

I saw that there's a class ContinuedFraction, maybe this class can
somehow be used within the constructor instead of duplicating broken
code between two classes, so I didn't bother extracting the above tests.

Also, I created a new branch and renamed the commits to include the
number of the JIRA ticket, which means that the revision numbers are now
different from those in the branch I previously referenced, but the
changes made in the commits are the same, in case anyone is confused.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

2019-06-20 Thread Heinrich Bohne


By the way, I've worked a bit on the draft in the meantime and pushed
the changes I've made so far, in case anyone is interested in
(re-)viewing them. Here's the link to the branch again:
https://github.com/Schamschi/commons-numbers/tree/FractionCommonTestCases

On 6/20/19 6:13 PM, Heinrich Bohne wrote:

Hello Eric,

I'm not sure if I understand what you mean by "context" when you say
that all context has to be within the unit test.Do you mean that the
test should not rely on the functionality of other
modules/methods/"units" than the one to be tested? If so, I agree with
you, but I don't think that this would be the case in this scenario.

The test methods in FractionTest would still only test the functionality
of Fraction, and those in BigFractionTest only that of BigFraction. It's
just that, in cases where similar/analogous functionality is tested,
they don't "come up" with test cases, i.e. combinations of input and
expected output values, themselves, but rather draw these from an
outside source that just happens to be queried by unit tests of a
different, entirely unrelated class as well.

The class FractionTest does not care that the class BigFractionTest
retrieves some of its test cases from the same source as FractionTest
does and vice versa. Likewise, the hypothetical class CommonTestCases
does not care what FractionTest or BigFractionTest do with the test
cases it provides, or if they use them at all. What matters is that
FractionTest and BigFractionTest each take responsibility for ensuring
that the test cases provided by CommonTestCases are applicable to the
functionality they want to test. Of course, this requires that
CommonTestCases clearly documents the test cases it provides. If, for
example, CommonTestCases were to provide test cases that are completely
useless to both FractionTest and BigFractionTest, then it would be the
responsibility of FractionTest and BigFractionTest to disregard them.

So the tests performed by FractionTest and BigFractionTest would not
rely on any functionality outside the unit they test, except for the
validity of the test cases. But erroneous test cases can happen
regardless of whether they are declared in the test class itself or
outside.


On 6/20/19 5:07 PM, Eric Barnhill wrote:

Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes
sense.
However from the perspective of unit tests it makes it no longer a unit
test.

IIUC it's best practice for a unit test that all context be within the
test. If additional context is required it fails to meet the
definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.

This results in unit tests often being clunkily and awkwardly coded,
but I
think it is the way they are typically written and it has its reasons.

  So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne 
wrote:


A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply
tests
and a few others just use values that would work with Fraction.
Possibly
these can be moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended
to be
complete, I just created it to give a general idea of how I would go
about
implementing this. I'll work some more on it before I create an
actual pull
request.


On 6/20/19 10:40 AM, Alex Herbert wrote:

On 20 Jun 2019, at 00:54, Heinrich Bohne 
wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only
differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only
implement
the test patterns.

I created a draft here:


https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f


Any opinions on this?

1. BigFraction should work the same way as Fraction when the
numbers are

the same

So collecting the common tests together makes sense. The change in the

PR looks good.

2. BigFraction should work with numbers that cannot be handled by

Fraction

A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply
tests
and a few others just use values that would work with Fraction.
Possibly
these can be moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure
the Big

part of BigFraction is working.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mai

Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

2019-06-20 Thread Heinrich Bohne


Hello Eric,

I'm not sure if I understand what you mean by "context" when you say
that all context has to be within the unit test.Do you mean that the
test should not rely on the functionality of other
modules/methods/"units" than the one to be tested? If so, I agree with
you, but I don't think that this would be the case in this scenario.

The test methods in FractionTest would still only test the functionality
of Fraction, and those in BigFractionTest only that of BigFraction. It's
just that, in cases where similar/analogous functionality is tested,
they don't "come up" with test cases, i.e. combinations of input and
expected output values, themselves, but rather draw these from an
outside source that just happens to be queried by unit tests of a
different, entirely unrelated class as well.

The class FractionTest does not care that the class BigFractionTest
retrieves some of its test cases from the same source as FractionTest
does and vice versa. Likewise, the hypothetical class CommonTestCases
does not care what FractionTest or BigFractionTest do with the test
cases it provides, or if they use them at all. What matters is that
FractionTest and BigFractionTest each take responsibility for ensuring
that the test cases provided by CommonTestCases are applicable to the
functionality they want to test. Of course, this requires that
CommonTestCases clearly documents the test cases it provides. If, for
example, CommonTestCases were to provide test cases that are completely
useless to both FractionTest and BigFractionTest, then it would be the
responsibility of FractionTest and BigFractionTest to disregard them.

So the tests performed by FractionTest and BigFractionTest would not
rely on any functionality outside the unit they test, except for the
validity of the test cases. But erroneous test cases can happen
regardless of whether they are declared in the test class itself or outside.


On 6/20/19 5:07 PM, Eric Barnhill wrote:

Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes sense.
However from the perspective of unit tests it makes it no longer a unit
test.

IIUC it's best practice for a unit test that all context be within the
test. If additional context is required it fails to meet the definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.

This results in unit tests often being clunkily and awkwardly coded, but I
think it is the way they are typically written and it has its reasons.

  So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne  wrote:


A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply tests
and a few others just use values that would work with Fraction. Possibly
these can be moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended to be
complete, I just created it to give a general idea of how I would go about
implementing this. I'll work some more on it before I create an actual pull
request.


On 6/20/19 10:40 AM, Alex Herbert wrote:

On 20 Jun 2019, at 00:54, Heinrich Bohne  wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:


https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are

the same

So collecting the common tests together makes sense. The change in the

PR looks good.

2. BigFraction should work with numbers that cannot be handled by

Fraction

A quick looks shows that the BigFractionTest does have test cases for

very large numbers. However the add, subtract, divide and multiply tests
and a few others just use values that would work with Fraction. Possibly
these can be moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure the Big

part of BigFraction is working.

-
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 uns

Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

2019-06-20 Thread Heinrich Bohne

A quick looks shows that the BigFractionTest does have test cases for very
large numbers. However the add, subtract, divide and multiply tests and a few
others just use values that would work with Fraction. Possibly these can be
moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended to be
complete, I just created it to give a general idea of how I would go about
implementing this. I'll work some more on it before I create an actual pull
request.

On 6/20/19 10:40 AM, Alex Herbert wrote:

On 20 Jun 2019, at 00:54, Heinrich Bohne wrote:

An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:
https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are the
same

So collecting the common tests together makes sense. The change in the PR looks
good.

2. BigFraction should work with numbers that cannot be handled by Fraction

Then variants added using BigInteger arguments just to make sure the Big part
of BigFraction is working.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[numbers-fraction] Code duplication between FractionTest and BigFractionTest

2019-06-19 Thread Heinrich Bohne


An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:
https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Migration to JUnit 5 complete

2019-06-18 Thread Heinrich Bohne


Hello,

Not that I'm aware of, so I created one:
https://issues.apache.org/jira/browse/NUMBERS-115. I hope I included the
reference to the pull request the correct way.

On 6/18/19 1:31 PM, Gilles Sadowski wrote:

Hi.

Le lun. 17 juin 2019 à 00:03, Heinrich Bohne  a écrit :

I made some additional refinements to the test classes in the
commons-numbers-field module (the one with the parameterized tests). The
compiler was warning about unchecked method calls and raw types, so I
added type parameters where appropriate to rectify these issues.

On 6/15/19 12:41 PM, Heinrich Bohne wrote:

Grimreaper and I finished work on the migration to JUnit 5 in the pull
request https://github.com/apache/commons-numbers/pull/48 , so the pull
request is ready for review.

Is there an associated JIRA report?

Gilles

-
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

Re: [numbers] Migration to JUnit 5 complete

2019-06-16 Thread Heinrich Bohne


I made some additional refinements to the test classes in the
commons-numbers-field module (the one with the parameterized tests). The
compiler was warning about unchecked method calls and raw types, so I
added type parameters where appropriate to rectify these issues.

On 6/15/19 12:41 PM, Heinrich Bohne wrote:

Grimreaper and I finished work on the migration to JUnit 5 in the pull
request https://github.com/apache/commons-numbers/pull/48 , so the pull
request is ready for review.

-
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

[numbers] Migration to JUnit 5 complete

2019-06-15 Thread Heinrich Bohne


Grimreaper and I finished work on the migration to JUnit 5 in the pull
request https://github.com/apache/commons-numbers/pull/48 , so the pull
request is ready for review.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Code blocks in test methods

2019-06-12 Thread Heinrich Bohne

> (2) Why not refactor and pull-out methods? This then forces you to _name_
> the methods, instead of the above (anonymous blocks vs. commented
blocks.)

I did not pull out the code sections into separate methods because I had no
intention of re-structuring the whole class. I only wanted to fix a bug in
the class Fraction and add a test case in FractionTest that would have
failed
due to this bug, and in theprocess organize that which was already
present in
FractionTest a bit better, because it has been pointed out to me that new
contributions should not only strive to improve functionality but also
readability.
Introducing those code blocks seemed like a straightforward way of
making the
mess in the bodies of some of the test methods in FractionTest more
comprehensible –
I would think that adding new methods could be more controversial than
adding
code blocks.

Besides, should anyone in the future wish to extract these code sections
into
separate methods, I doubt that the code blocks would be a hindrance – if
anything,
I imagine that they would it easier, because with the code blocks, it is
a lot
clearer WHAT can be extracted to a method in the first place than
without them.

> (1) It is helpful to add a // comment for each block, otherwise, it
feels
> anonymous and weird to me.

I am not sure how adding a comment to the code blocks would be helpful in
this case. The blocks only serve to separate the test cases, and most of
the time,
these test cases differ only in the values that they use, and not in
some defining
characteristic. For example, the first three blocks in testReciprocal()
only test
some different arbitrary fractions, but are otherwise completely
analogous, so I
couldn't think of any other comment than something like "test case 1",
"test case 2",
etc. Granted, the fourth block tests failure with a zero-denominator, so
in this
case, I can understand your point about adding comments. By the way,
some of these
test cases were already commented before I edited the class.

On 6/12/19 3:08 PM, Gary Gregory wrote:

I've used code blocks in this style in the past but...

(1) It is helpful to add a // comment for each block, otherwise, it feels
anonymous and weird to me.
(2) Why not refactor and pull-out methods? This then forces you to _name_
the methods, instead of the above (anonymous blocks vs. commented blocks.)

Gary

On Wed, Jun 12, 2019 at 9:00 AM Heinrich Bohne 
wrote:

I have been asked to request some feedback on this pull request:
https://github.com/apache/commons-numbers/pull/36– specifically, about
the introduction of code blocks in the commit "NUMBERS-100: Reduce scope
of local variables".

I had the idea with the code blocks when I wanted to add a test to the
method testAdd() but was intimidated by the huge wall of code contained
in the method. When taking a closer look, this code wall is actually
composed of several test cases that are completely independent of each
other, but because the local variables live throughout the whole method
and are re-used in almost every test case, this is not obvious. The more
variables are involved, the closer you have to look to figure out which
sections are independent of the rest.

I think that, with the code blocks, it is instantly obvious that a
specific section does not depend on anything that happened before it, or
that it does not affect anything that comes after it. So I think that
they are preferable to the previous version of the file.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [numbers] Code blocks in test methods

2019-06-12 Thread Heinrich Bohne


Sorry, I messed up the link to the pull request:
https://github.com/apache/commons-numbers/pull/36

On 6/12/19 3:00 PM, Heinrich Bohne wrote:

I have been asked to request some feedback on this pull request:
https://github.com/apache/commons-numbers/pull/36– specifically, about
the introduction of code blocks in the commit "NUMBERS-100: Reduce
scope of local variables".

I had the idea with the code blocks when I wanted to add a test to the
method testAdd() but was intimidated by the huge wall of code
contained in the method. When taking a closer look, this code wall is
actually composed of several test cases that are completely
independent of each other, but because the local variables live
throughout the whole method and are re-used in almost every test case,
this is not obvious. The more variables are involved, the closer you
have to look to figure out which sections are independent of the rest.

I think that, with the code blocks, it is instantly obvious that a
specific section does not depend on anything that happened before it,
or that it does not affect anything that comes after it. So I think
that they are preferable to the previous version of the file.



-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[numbers] Code blocks in test methods

2019-06-12 Thread Heinrich Bohne


I have been asked to request some feedback on this pull request:
https://github.com/apache/commons-numbers/pull/36– specifically, about
the introduction of code blocks in the commit "NUMBERS-100: Reduce scope
of local variables".

I had the idea with the code blocks when I wanted to add a test to the
method testAdd() but was intimidated by the huge wall of code contained
in the method. When taking a closer look, this code wall is actually
composed of several test cases that are completely independent of each
other, but because the local variables live throughout the whole method
and are re-used in almost every test case, this is not obvious. The more
variables are involved, the closer you have to look to figure out which
sections are independent of the rest.

I think that, with the code blocks, it is instantly obvious that a
specific section does not depend on anything that happened before it, or
that it does not affect anything that comes after it. So I think that
they are preferable to the previous version of the file.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[numbers] Redundant methods in ArithmeticUtils

2019-06-11 Thread Heinrich Bohne


The class ArithmeticUtils in the commons-numbers-core module contains
several methods where, since Java 8, equivalent methods in
java.lang.Math exist. These methods are the following:

addAndCheck(int, int)
addAndCheck(long, long)
mulAndCheck(int, int)
mulAndCheck(long, long)
subAndCheck(int, int)
subAndCheck(long, long)

The corresponding methods from java.lang.Math are:

addExact(int, int)
addExact(long, long)
multiplyExact(int, int)
multiplyExact(long, long)
subtractExact(int, int)
subtractExact(long, long)

The former methods are probably relics from pre-Java-8 times, when the
latter methods did not exist. But now, they are redundant. I suggest
they be removed from ArithmeticUtils in commons-numbers-core, and their
invocations replaced by invocations of the java.lang.Math equivalents.
Both groups of methods specify the same type of exception to be thrown
in case of an overflow (a java.lang.ArithmeticException), so the
replacement should be straightforward.

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: * GMX Spamverdacht * Aw: Re: Proposal to introduce JUnit 5 in commons-numbers

2019-06-04 Thread Heinrich Bohne


So, if the migration from JUnit 4 to JUnit 5 has to happen
instantaneously, how about creating a new branch for the migration? It's
a lot of files that have to be refactored, which is going to take some
time, and since the changes will need to be reviewed, it would probably
be easier if the review can already take place while the migration is in
progress, rather than all at once when it is finished. Also, it would
reduce the risk of conflicting pull requests, thus potentially saving time.

On 5/27/19 8:28 AM, Sven Rathgeber wrote:

+1

We did the migration 4 -> 5 for various products at work. It was 
straightforward.
I vote for a clean cut, as well.

s/Before/BeforeEach/
s/After/AfterEach
 and you are halfway through.

Sven

Gesendet: Montag, 27. Mai 2019 um 07:28 Uhr
Von: "Eitan Adler" 
An: "Heinrich Bohne" 
Cc: "Commons Developers List" 
Betreff: Re: Proposal to introduce JUnit 5 in commons-numbers
(please make sure to CC me directly on replies)

On Sun, 26 May 2019 at 17:17, Heinrich Bohne  wrote:


... (I didn't subscribe to the list, which I now regret as I am editing
the header fields of this email manually, hoping that I do it correctly and
that the message will be attributed to the correct thread).


This is the first time I seeing this thread.



Regarding the point about a potential mess of multiple JUnit versions:
JUnit 5 seems to provide a "Vintage" test engine to support the execution
of JUnit 3 and 4 tests alongside JUnit 5 tests.


It is precisely this that I am advocating against. While JUnit 5 has a
method of running older tests, by leaving the older style tests around
we're producing confusion as to how it will get run or how to write new
tests. Using it as a migration tool is reasonable, but the commons are
small enough that wholesale migration is "easy".





--
Eitan Adler







-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[numbers-primes] Improving trial division code and algorithm

2019-06-03 Thread Heinrich Bohne


I have been advised to raise this improvement suggestion on the
developers' mailing list. It is about the trial division algorithm in
the method SmallPrimes.boundedTrialDivision(int, int, List) in
the primes module. Currently, this algorithm skips multiples of 2 and 3
as trial candidates, which reduces the number of integers to be tried to
1/3 of all integers. The choice of these two prime numbers as the only
ones whose multiples should be skipped seems arbitrary and is probably
rooted in the fact that the way the code currently achieves this is
based on code duplication (by hard-coding the alternation of the trial
candidate's increment between 2 and 4), and choosing any other prime
number, or more prime numbers, would increase the extent of the code
duplication to insufferable dimensions.

However, when altering the code's mechanism not to rely on code
duplication, using more primes than just 2 and 3 is conceivable. For
example, when skipping multiples of 2, 3, 5, 7 and 11, the number of
integers to be tried can be reduced to 16/77 of all integers, at the
cost of storing 480 pre-computed integers in an array. Considering that
the class SmallPrimes already contains an array with the first 512 prime
numbers, this does not seem like very much.

Some more details about this suggestion are explained here:
https://issues.apache.org/jira/browse/NUMBERS-104

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: Proposal to introduce JUnit 5 in commons-numbers

2019-05-26 Thread Heinrich Bohne

Hello, and excuse me for the late reply, I didn't realize until now that
the online archive of the mailing list can have more than one page per
month, and since my original email didn't show up on the first page, I
was wondering whether it ever went through (I didn't subscribe to the
list, which I now regret as I am editing the header fields of this email
manually, hoping that I do it correctly and that the message will be
attributed to the correct thread).

Regarding the point about a potential mess of multiple JUnit versions:
JUnit 5 seems to provide a "Vintage" test engine to support the
execution of JUnit 3 and 4 tests alongside JUnit 5 tests. If this
Vintage test engine is added as a dependency in a Maven project in
addition to JUnit 4, then Maven will be able to run JUnit 4 tests on the
Vintage engine whilst running JUnit 5 tests on the Jupiter engine from
JUnit 5 (provided the Jupiter engine is added as a dependency). See
https://junit.org/junit5/docs/current/user-guide/#running-tests-build-maven
from the JUnit 5 user guide.

I also tried it without the Vintage engine, i.e. only JUnit 4 and the
Jupiter engine, but then, Maven would either run only the JUnit 4 tests
or only the JUnit 5 tests, or sometimes no tests at all, who knows why.
But with the Vintage engine, new tests can be written in JUnit 5 without
requiring all existing JUnit 4 tests to be migrated to JUnit 5 at once,
because, as far as I understand it, JUnit 5 tests will be run by the
Jupiter engine, JUnit 4 tests can be run by the Vintage engine until
they are migrated to JUnit 5, and the JUnit 4 dependency, it seems to
me, is only needed for compiling the JUnit 4 tests, but not for running
them.

--

From    Gilles Sadowski 
Subject    Re: Proposal to introduce JUnit 5 in commons-numbers
Date    Sat, 25 May 2019 10:11:43 GMT

Hi.

Le ven. 24 mai 2019 à 06:01, Eitan Adler  a écrit :

(please make sure to CC me on replies)

+1 on this. One thing I'd like for us to avoid a mess of different junit
versions making it difficult to know which runner will be executing the
class. It would be great if we did a complete conversion and not just
introduced new syntax.

+1

I've actually done this before on a largish Java
project from JUnit3/4 to 5. It isn't hard, just a fair amount of mechanical
code changes.

Patch/PR welcome.

Regards,
Gilles

On Wed, 22 May 2019 at 15:30, Eric Barnhill  wrote:

> +1
>
> On Wed, May 22, 2019 at 3:15 PM Gilles Sadowski 
> wrote:
>
> > Hi.
> >
> > Le mer. 22 mai 2019 à 18:43, Heinrich Bohne 

a

> > écrit :
> > >
> > > Right now, commons-numbers is using JUnit 4.12, the last stable version
> > > of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
> > > 4.12 for testing whether an exception is thrown apart from either using
> > > the deprecated class ExpectedException or adding the "expected"
> > > parameter to the Test annotation. The problem with the latter approach
> > > is that it is impossible to ascertain where exactly in the annotated
> > > method the exception is thrown – it could be thrown somewhere
> unexpected
> > > and the test will still pass. Besides, when testing the same exception
> > > trigger with multiple different inputs, it is impractical to create a
> > > separate method for each test case, which would be necessary with both
> > > aforementioned approaches.
> > >
> > > This has led to the creation of constructs where the expected exception
> > > is swallowed, which has been deemed undesirable
> > > <
> >
> 
https://issues.apache.org/jira/browse/NUMBERS-99?focusedCommentId=16843419=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16843419
> > >.
> > > Because of this, I propose to add JUnit 5 as a dependency in
> > > commons-numbers. JUnit 5 has several "assertThrows" methods that would
> > > solve the described dilemma.
> >
> > +1
> >
> > Gilles
> >

Proposal to introduce JUnit 5 in commons-numbers

2019-05-22 Thread Heinrich Bohne


Right now, commons-numbers is using JUnit 4.12, the last stable version
of JUnit 4. As far as I am aware, there is no explicit syntax in JUnit
4.12 for testing whether an exception is thrown apart from either using
the deprecated class ExpectedException or adding the "expected"
parameter to the Test annotation. The problem with the latter approach
is that it is impossible to ascertain where exactly in the annotated
method the exception is thrown – it could be thrown somewhere unexpected
and the test will still pass. Besides, when testing the same exception
trigger with multiple different inputs, it is impractical to create a
separate method for each test case, which would be necessary with both
aforementioned approaches.

This has led to the creation of constructs where the expected exception
is swallowed, which has been deemed undesirable
.
Because of this, I propose to add JUnit 5 as a dependency in
commons-numbers. JUnit 5 has several "assertThrows" methods that would
solve the described dilemma.

Re: [numbers-core] Special cases in greatest common divisor methods

[numbers-core] Special cases in greatest common divisor methods

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

Re: [numbers-fraction] Double approximation constructor/factory method overhaul

[numbers-fraction] Double approximation constructor/factory method overhaul

Re: False coverage decrease accusations by Coveralls

Re: False coverage decrease accusations by Coveralls

Re: False coverage decrease accusations by Coveralls

Re: False coverage decrease accusations by Coveralls

False coverage decrease accusations by Coveralls

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

Re: [numbers-fraction] Documentation of fractions' reduction to lowest terms

[numbers-fraction] Documentation of fractions' reduction to lowest terms

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Re: Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

[numbers-fraction] Code duplication between FractionTest and BigFractionTest

Re: [numbers] Migration to JUnit 5 complete

Re: [numbers] Migration to JUnit 5 complete

[numbers] Migration to JUnit 5 complete

Re: [numbers] Code blocks in test methods

Re: [numbers] Code blocks in test methods

[numbers] Code blocks in test methods

[numbers] Redundant methods in ArithmeticUtils

Re: * GMX Spamverdacht * Aw: Re: Proposal to introduce JUnit 5 in commons-numbers

[numbers-primes] Improving trial division code and algorithm

Re: Proposal to introduce JUnit 5 in commons-numbers

Proposal to introduce JUnit 5 in commons-numbers

33 matches

Site Navigation

Mail list logo

Footer information