On Mon, 4 Jul 2022 at 23:00, Sumanth Rajkumar <[email protected]>
wrote:
> Thanks Alex for PR feedback.
> I have incorporated the majority of the requested changes to the PR.
>
> I would like to discuss the remaining points here.
>
> > In
>
> commons-numbers-complex/src/main/java/org/apache/commons/numbers/complex/ComplexUnaryOperator.java:
> > Note that by making this a default that creates an instance of Complex
> you impose memory allocation overhead to any call site that just has the
> real and imaginary parts (e.g. a structure storing a list of complex
> numbers using primitive arrays).
> > The ComplexDouble apply(ComplexDouble in,
> ComplexConstructor<ComplexDouble> out) method should be a default that
> calls this function using the real and imaginary parts.
>
> The existing unit test for Complex Projection (proj) is expecting the
> returned result to be the same as the passed input Complex instance except
> for the isInfinite edge case.
> This was the main reason for making the functional interface accept a
> ComplexDouble instead of primitive real and imaginary parts.
>
Passing an existing unit test should not govern a design decision.
The proj method will return the same number if it is not an infinite
complex number. That is all that is required to pass the test, the input
and output should have the exact same real and imaginary parts. Note that
the Complex class is immutable so it can return itself. The test for object
identity can be dropped in a generic case where the output is passed to the
ComplexConstructor.
>
> The additional memory allocation for the input can be eliminated (for the
> example of primitive array backed lists), by combining it with the
> constructed result object.
> We had decided to incur the overhead for the complex Constructor
> result object in the context of functional compositions and thread safety
> discussion before.
> So, passing in a cursor iterator item (that captures the index) as both
> ComplexDouble input and ComplexResult constructor should avoid any
> additional overhead?
>
In this case yes. But in the case of usage outside of the complex package,
for example if the raw real and imaginary arrays are created by a 2D FFT.
At present I do not see a downside to the functional method being the
separate arguments of real and imaginary primitives, other than it
decouples the values and so is not encapsulated. Let's see what others
think.
>
> If we decide to use primitive types for the functional interface, then we
> cannot use it for the projection method.
>
Not a problem. Just write a dedicated method for Complex and a generic
method in ComplexFunctions. This small level of code duplication is
acceptable where the efficiency of the method is greatly improved by
rewriting it, as has been done for the scalar functions.
>
>
> > @FunctionalInterface
> > public interface ComplexScalarFunction {
> >
> > ComplexDouble apply(ComplexDouble c, double f,
> ComplexConstructor<ComplexDouble> result);
> >
> > }
> > This interface should be typed: the result is accepted by the
> ComplexConstructor and this can be typed.
>
>
> By typed, did you mean to make this a generic interface
> ComplexScalarFunction<T>
> ?
> ComplexUnaryOperator and ComplexBinaryOperator are not generic and
> constrained to ComplexDouble types
>
Why should I have to return a ComplexDouble? Since the ComplexConstructor
is typed it makes more sense to have the methods that use it to also be
typed. These methods should perform an operation that generates a real and
imaginary part. This is passed to the provided constructor. It should be up
to the provider of the constructor (i.e. the caller) to decide what to do
with the result. By typing to ComplexDouble you are removing that
flexibility.
ComplexUnaryOperator extends UnaryOperator<ComplexDouble>
I do not see what extending UnaryOperator provides. It constrains the
interface to having to support composition imposed by Function<T, R>. Note
that composition can avoid the Complex constructor by directly chaining the
output to the next input. Consider this simplification:
public interface ComplexUnaryOperator<R> {
R apply(double r, double i, ComplexConstructor<R> out);
default ComplexUnaryOperator<R> andThen(ComplexUnaryOperator<R> after) {
Objects.requireNonNull(after);
return (r, i, out) -> apply(r, i, (x, y) -> after.apply(x, y, out));
}
}
public interface ComplexBinaryOperator<R> {
R apply(double r1, double i1, double r2, double i2,
ComplexConstructor<R> out);
default ComplexBinaryOperator<R> andThen(ComplexUnaryOperator<R> after)
{
Objects.requireNonNull(after);
return (r1, i1, r2, i2, out) -> apply(r1, i1, r2, i2, (x, y) ->
after.apply(x, y, out));
}
}
Note that this has some differences from java.util.function during
composition.
In the generic case of java.util.function an object is returned. This is
passed to the next function and may be changed to an object of a different
type. So a composition of Function<T, R> and Function<R, V> becomes
Function<T, V> (via T -> R -> V).
In the complex function case the output will be a (real, imaginary) pair;
this is defined by the requirement to support the ComplexConstructor<R> for
the output. All functions produce this output. Thus during composition the
generic type R specifies the terminating object type. All intermediates are
a primitive pair and no objects are created (lambda functions are created
to channel the values through the composed function). Thus each composition
maintains the same generic type <R> as that specifies the ultimate
destination of the complex number.
However there is no requirement to specify what R is, allowing it to be
Void. This also decouples the interface from Complex and ComplexDouble.
Note that this may make ComplexDouble obsolete which would simplify the
current changes.
I've not applied this change to your entire current diff so there may be
some issues, for example with the scalar functions. I am interested to see
if this would work.
>
>
> > It may be wise to update the test suite to ensure that all tests
> currently applied to Complex are applied to ComplexFunctions using a
> ComplexConstructor other than Complex, for example using a dummy
> implementation:
> >
> > class ComplexNumber implements ComplexConstructor<ComplexNumber> {
> > // (r, i) members
> >
> > @Override
> > public ComplexNumber apply(double r, double i) {
> > // store (r, i) ...
> > return this;
> > }
> > }
> >
> >This will detect the edge cases all pass through to the input constructor
> to create the result. The test should assert the ComplexConstructor
> received the expected values.
>
> For now, shall I copy all the tests from the CStandardTest, CReferenceTest
> and ComplexEdgeCaseTest to ComplexFunctionsTest and modify it to use a
> dummy constructor?
> Also please note as mentioned above for the complex.proj method, currently
> we are not invoking the complex constructor for normal cases.
>
The CReferenceTest is simple to modify since it loads (re, im) pairs for
the input and output. For example just update the functions to test both
Complex and ComplexFunctions at the same time:
private static void assertOperation(String name,
UnaryOperator<Complex> operation, long maxUlps) {
to
private static void assertOperation(String name,
UnaryOperator<Complex> operation,
ComplexUnaryOperator<ComplexResult> operation2, long maxUlps) {
with a ComplexResult suitably created to check the result pair is passed to
the result constructor. These assertions should check the (real, imag) pair
in complex is an exact match for the (real, imag) pair passed to the
ComplexResult generated by the constructor, i.e. they are not just equal to
the expected result within a tolerance, as each could be a different result
and still be within tolerance of the expected.
The same can be done for the CStandardTest and the ComplexEdgeCaseTest as
these also test functions using a generic assertion method which accepts
the function to test.
As for the ComplexTest this is harder to refactor. But what is required is
that all tests currently present in ComplexTest should be applied to the
Complex and ComplexFunctions methods. Perhaps start a new test class that
copies ComplexTest and then tests the results using a generic assert method
that accepts two operations: one on Complex, and one on primitive pairs.
This would highlight the current lack of functionality in ComplexFunctions,
for example the subtractFromImaginary function.
Note that in looking for coverage it is useful to be able to run part of
the test suite and generate a report [1]:
mvn clean
mvn test -Dtest=CStandardTest
mvn test -Dtest=CStandardTest#testAcos (single method)
mvn jacoco:report
open target/site/jacoco/index.html
You should be able to see the coverage of everything that has been run
since the last 'mvn clean' command.
Alex
[1]
https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#test
