Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numbers 188: refactored binary functions #119

Conversation

sumanth-rajkumar
Copy link
Contributor

This PR refactors Complex class using functional interfaces and static methods.
This allows reuse of the refactored static functions for Operations on List/Matrices of Complex Numbers (NUMBERS-186)

The changes retain binary backward compatibility.

Summary of changes

  1. Introduced following interfaces for Complex Number Operations

ComplexBinaryOperator - binary operations on complex numbers represented as double real and imaginary parts

  1. Refactored all binary operations instance methods of Complex class as static functions in ComplexFunctions
    The static functions use the functional interface signatures described above

Copy link
Contributor

@aherbert aherbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sumanth-rajkumar,

The changes look good. There are a few javadoc issues to correct.

If you perform these changes you can branch from your updated PR code (i.e. the latest point of development) and then start on refactoring the remaining Complex-scalar argument binary functions. I should be able to merge this PR into the feature branch and your next PR can simply rebase from the feature branch.

* @param after the function to apply after this function is applied
* @return a composed function that first applies this function and then applies the after function
*/
default ComplexBinaryOperator<R> andThen(ComplexUnaryOperator<R> after) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not currently required


/**
* Represents an operator that accepts real and imaginary parts of a complex number and supplies the complex result to the provided consumer.
* @param real1 Real part \( a \) of the first complex number \( (a +ib) \).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parts of two complex numbers

Add a blank line before the param tags. You can do the same in the other interface for consistency.

*/
public static <R> R add(double re1, double im1, double re2, double im2, ComplexSink<R> action) {
return action.apply(re1 + re2,
im1 + im2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

*/
public static <R> R subtract(double re1, double im1, double re2, double im2, ComplexSink<R> action) {
return action.apply(re1 - re2,
im1 - im2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent

}

/**
* Returns a {@code Object} whose value is:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong javadoc. Copy from Complex which uses the MathJax formula.

Note that the use of <pre> tags for formulas was only used on internal private methods. This can be rendered by an IDE allowing for example you to read the javadocs when hovering over a method.

Public API javadoc should all use MathJax. This is rendered when creating the docs. See:

mvn javadoc:javadoc
open target/site/apidocs/index.html

@@ -162,7 +162,7 @@ static void assertEquals(Supplier<String> msg, double expected, double actual, l
* @param operation1 Operation on the Complex object.
* @param operation2 Operation on the complex real and imaginary parts.
* @param expected Expected result.
* @param maxUlps Maximum units of least precision between the two values.
* @param maxUlps Maximum units of the least precision between the two values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wikipedia does not agree here: ULP

Can you change all javadoc back to units of least precision

* @return the object returned by the supplied action.
* @see <a href="http://mathworld.wolfram.com/ComplexAddition.html">Complex Addition</a>
*/
public static <R> R add(double re1, double im1, double re2, double im2, ComplexSink<R> action) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be very verbose but for consistency we should use real1/2 and imaginary1/2 as these are the names used for all the unary functions. This matches what you have put in the javadoc too.

* @param im1 Imaginary part \( b \) of the first complex number \( (a +ib) \).
* @param re2 Real part \( a \) of the second complex number \( (a +ib) \).
* @param im2 Imaginary part \( b \) of the second complex number \( (a +ib) \).
* @param action Consumer for the final multiplied complex number.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumer for the multiplication result.

* @param im1 Imaginary part \( b \) of the first complex number \( (a +ib) \).
* @param re2 Real part \( a \) of the second complex number \( (a +ib) \).
* @param im2 Imaginary part \( b \) of the second complex number \( (a +ib) \).
* @param action Consumer for the final divided complex number.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumer for the division result.

* @param im1 Imaginary part \( b \) of the first complex number \( (a +ib) \).
* @param re2 Real part \( a \) of the second complex number \( (a +ib) \).
* @param im2 Imaginary part \( b \) of the second complex number \( (a +ib) \).
* @param action Consumer for the power of the complex number.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumer for the power result.

@codecov-commenter
Copy link

Codecov Report

Merging #119 (541e601) into complex-gsoc-2022 (1b0bfa6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##             complex-gsoc-2022     #119   +/-   ##
====================================================
  Coverage                99.12%   99.13%           
- Complexity                1676     1684    +8     
====================================================
  Files                       65       65           
  Lines                     4252     4259    +7     
  Branches                   834      835    +1     
====================================================
+ Hits                      4215     4222    +7     
  Misses                      10       10           
  Partials                    27       27           
Impacted Files Coverage Δ
...va/org/apache/commons/numbers/complex/Complex.java 100.00% <100.00%> (ø)
...ache/commons/numbers/complex/ComplexFunctions.java 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

@aherbert aherbert merged commit 7386b09 into apache:complex-gsoc-2022 Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants