Hi.
Le sam. 9 nov. 2019 à 01:48, Alex Herbert <alex.d.herb...@gmail.com> a écrit :
>
>
>
> > On 7 Nov 2019, at 23:29, Eric Barnhill <ericbarnh...@gmail.com> wrote:
> >
> > On Thu, Nov 7, 2019 at 3:25 PM Gilles Sadowski <gillese...@gmail.com> wrote:
> >
> >> Le jeu. 7 nov. 2019 à 18:36, Eric Barnhill <ericbarnh...@gmail.com> a
> >> écrit :
> >>>
> >>> I should also add on this note, my use case for developing ComplexUtils
> >> in
> >>> the first place was compatibility with JTransforms and JOCL. In both
> >> cases
> >>> I wanted to convert Complex[] arrays into interleaved double[] arrays to
> >>> feed into algorithms using those libraries.
> >>
> >> Implicit in my remark below is the question: Where does the "Complex[]"
> >> come from? If it is never a good idea to create such an array, why provide
> >> code to convert from it? Do we agree that we should rather create the
> >> "ComplexList" abstraction, including accessors that shape the data for
> >> use with e.g. JTransforms?
> >>
> >>
> > I completely agree this is a superior solution and look forward to its
> > development.
>
> I think at least the minimum implementation of the abstraction will be fairly
> easy. It can then be performance checked with a few variants.
>
> There currently is not a JMH module for numbers. Should I create one under a
> module included with an optional profile?
What is going to be benchmarked?
>
> I have spent a fair bit of time trying to fix coverage of Complex. It is now
> at 100% with 95% branch coverage. It currently tests all the edge cases for
> the C standard. However it doesn’t really test actual computations.
Actual computations?
Coveralls seems OK:
https://coveralls.io/builds/26869201/source?filename=commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/Complex.java
> I am going to continue on this to add more tests that the computations output
> the same values as a C reference implementation, and hit all the branches.
>
> When working on the class it raised a few questions:
>
> 1. HashCode
>
> This is current the same for any number with a NaN component. However the C
> standard states that NaN components are allowed in infinites. So perhaps the
> hashCode should be updated to at least distinguish infinites from NaN.
What would be the purpose?
>
> 2. NaN definition
>
> What is not clear is if there exists a definition of NaN for a complex
> number. The C library has no way to test it with a single function call. You
> have to test both the real and imaginary components. On top of this you can
> do computation with NaNs. The GNU g++ compiler computes:
>
> (1e300 + i 1e300) * (1e30 + i NAN) = inf + i inf
>
> Thus this is allowing some computations with NaN to produce results. I don’t
> know if this is a bug in GNU g++ or intentional. The C standard states:
>
> “If both operands of the * operator are complex or if the second operand of
> the / operator
> is complex, the operator raises floating-point exceptions if appropriate for
> the calculation
> of the parts of the result, and may raise spurious floating-point exceptions.”
>
> That is pretty vague. It says that you can do what is “appropriate”.
>
> So it seems that perhaps we should only call a complex a NaN is both fields
> are NaN. If only one is NaN then the complex is semi-NaN and results of
> computations may or may not make sense.
Do you have an example of a NaN that makes sense?
What is semi-NaN?
>
> 3. Multiplication
>
> I applied a change to the algorithm provided in the C standard when
> recovering (NaN,NaN) results of multiplying infinities. If you multiply
> infinity by zero this should be NaN.
>
> “if one operand is an infinity and the other operand is a nonzero finite
> number or an
> infinity, then the result of the * operator is an infinity;"
>
> Note the “non-zero” condition.
>
> But the algorithm provided does not do this and you can get an infinite
> result back.
>
> This is what GNU g++ does:
>
> multiply(0 + i 0, 0 + i inf) = -nan + i -nan
> multiply(-0 + i 0, 0 + i inf) = -nan + i -nan
> multiply(0 + i 0, -0 + i inf) = -nan + i -nan
> multiply(-0 + i 0, -0 + i inf) = -nan + i -nan
> multiply(0 + i 0, 1 + i -inf) = -nan + i -nan
> multiply(-0 + i 0, 1 + i -inf) = -nan + i -nan
>
> So they have implemented the multiply differently because here multiply
> infinity by zero is NaN. Note how printf distinguishes the sign of infinite
> values. I don’t think Java does this. It collates NaN into one
> representation. I will have to convert to raw long bits to get the sign for a
> cross reference to a C version.
I don't follow. Why would it be useful to distinguish -NaN from NaN or i NaN?
>
> 4. Divide
>
> I have applied a change to this algorithm to check for overflow in the
> division. The reference text states:
>
> “if the first operand is a finite number and the second operand is an
> infinity, then the
> result of the / operator is a zero;”
>
> But for example GNU g++ does this:
>
> divide(1e+308 + i 1e+308, inf + i inf) = -nan + i 0
>
> This is what the reference algorithm does if it is not changed. Here overflow
> cause problems in the result (which should be zero). So I think my patch is
> correct and the result should be (0 + i 0).
Not sure; I'd think that division by an imaginary infinity should yield NaN.
> The current unit tests are enforcing this requirement.
>
> 5. Edge cases
>
> Some of the methods have a large amount of if statements to test edge cases.
> These are very verbose and hard to follow. They also leave the common path of
> a non infinite, non nan, non zero value at the end. So a rearrangement may
> allow better branch prediction for the common use case. I’ll carry on working
> on this as I add more tests.
>
> 6. tanh
>
> I have put a TODO: note in the code about this. It tests edge case for
> infinites. Then doubles the real and imaginary components. These could then
> overflow to infinite and so trigger the edge case that has just been skipped.
> I am going to compare the result to the GNU C version to decide how to handle
> overflow.
Thanks for the review!
>
> Numbers overall
>
> I’ve now updated the configs for checkstyle, PMD and spotbugs.
> Checkstyle:check
How to disable "failOnViolation" from the command line?
> and spotbugs:check are enabled. Pmd:check is not. There are some loose ends
> to fix for that. Unfortunately the syntax for exclusions in the PMD config
> file are poor. I’ve added some but I’ve not looked at everything yet. A few
> decisions might need to be taken on whether to drops rules rather than fix
> them to at least get pmd:check active. It spots a lot of things so is useful
> for PRs.
Maybe. But how does one know what to fix when the build fails because
of a CheckStyle
or PMD violation?
> I’ve also not looked at SonarCloud [1]. There are a few things on there to
> fix too.
Most (all?) are false positives.
> [1] https://sonarcloud.io/dashboard?id=commons-numbers
> <https://sonarcloud.io/dashboard?id=commons-numbers>
Best,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
