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

Add complex number support to log #514

Merged
merged 5 commits into from
Dec 13, 2022
Merged

Add complex number support to log #514

merged 5 commits into from
Dec 13, 2022

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Nov 21, 2022

This PR

  • adds complex number support to log by documenting special cases. By convention, the natural logarithm has a single branch cut, which is defined as the real interval (-infinity, 0).
  • updates the input and output array data types to be any floating-point data type, not just real-valued floating-point data types.
  • derives special cases from C99

@kgryte kgryte added API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types. labels Nov 21, 2022
@kgryte kgryte added this to the v2022 milestone Nov 21, 2022
@rgommers
Copy link
Member

Sphinx is unhappy with the branch cuts label:

/home/circleci/repo/spec/API_specification/array_api/elementwise_functions.py:docstring of
array_api.elementwise_functions.log:41: WARNING: undefined label: branch-cuts

@kgryte
Copy link
Contributor Author

kgryte commented Nov 21, 2022

@rgommers This is because we need to merge #461 first. The other branch cut PRs are dependent on that PR due to the added/referenced design doc.

@kgryte
Copy link
Contributor Author

kgryte commented Nov 27, 2022

Now that #461 is merged, all checks for this PR now pass.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @kgryte. One minor comment on the diff. My main comment is about the branch cuts - it would be very helpful to document that they are consistent with (and based on) those from C99. It is noted in the branch cuts design document, however that is probably easy to miss (and it requires the user to know which functions are in C99 and which aren't). Maybe in a couple of words, "branch cuts for log match those in C99"?.

@asmeurer
Copy link
Member

asmeurer commented Dec 1, 2022

For functions like this with a branch cut, should we add special cases along the cut, i.e., a + 0j and a - 0j where a < 0?

Co-authored-by: Aaron Meurer <[email protected]>
@kgryte
Copy link
Contributor Author

kgryte commented Dec 1, 2022

@asmeurer
Copy link
Member

asmeurer commented Dec 1, 2022

Is it worth specially noting that functions like log, sqrt, etc. are not expected to promote floating-point arguments outside of their real domain to complex (and indeed, such value-based casting is explicitly recommended against)?

@asmeurer
Copy link
Member

asmeurer commented Dec 1, 2022

@asmeurer That should already be covered by #514 (files).

Did you mean to link to a specific line? I don't see where this is mentioned.

@kgryte
Copy link
Contributor Author

kgryte commented Dec 1, 2022

Did you mean to link to a specific line?

From your comment, seemed like you were referring to how complex conjugates should be specified.

@kgryte
Copy link
Contributor Author

kgryte commented Dec 1, 2022

Is it worth specially noting that functions like log, sqrt, etc. are not expected to promote floating-point arguments outside of their real domain to complex (and indeed, such value-based casting is explicitly recommended against)?

Yes, this is likely a good idea. Probably better to include as part of the general complex number design doc.

@asmeurer
Copy link
Member

asmeurer commented Dec 2, 2022

I mean specifically special cases along the cut, for +0 and -0. Something like "If a is negative and b is +0.0, log(a + bj) should equal log(-a) + π" and "If a is negative and b is -0.0, log(a + bj) should equal log(-a) - π". That's why I asked in the last meeting if all the cuts where either on the real or imaginary axes, because when they are you can always pick a side of the cut with positive or negative 0.

Also, a weird thing I just discovered: 1-0.0j doesn't create a complex number with -0.0 imaginary part. You have to do -(-1+0j) or complex(1, -0.0).

@rgommers
Copy link
Member

rgommers commented Dec 5, 2022

My main comment is about the branch cuts - it would be very helpful to document that they are consistent with (and based on) those from C99. It is noted in the branch cuts design document, however that is probably easy to miss (and it requires the user to know which functions are in C99 and which aren't). Maybe in a couple of words, "branch cuts for log match those in C99"?.

This comment applies to a large number of open PRs, so I will add it as a separate task in gh-533.

@kgryte
Copy link
Contributor Author

kgryte commented Dec 12, 2022

@asmeurer At the moment, the spec does cover the special cases you mention. Per the guidance in C99 (and also included in this PR), log(conj(z)) == conj(log(z)). In which case, if $z = a + bj$ with $a &lt; 0$

z = a + 0j;
log(z) = log(a) + πj
conj(log(z)) = conj(log(a) + πj) = log(a) - πj
log(conj(z)) = log(a - 0j) = log(a) - πj            // per equality requirement

We can see that the equality requirement holds by computing the principal value of the complex logarithm in terms of atan2.

$$\log(z) = \log(r) + \theta j = \log(|z|) + j \operatorname{Arg}(z) = \log(\sqrt{a^2 + b^2}) + j \operatorname{atan2}(b,a)$$

where $z = a + bj = r e^{\theta j}$ and $r = |z| = \sqrt{a^2 + b^2}$. If we plug in the values for $a &lt; 0$ and $b = \pm 0$, we get

log(a + 0j) = log(a) + j atan2(+0, a) = log(a) + πj
log(a - 0j) = log(a) + j atan2(-0, a) = log(a) - πj

In short, I don't think we need to go beyond what this PR already does. Namely, require that log(conj(z)) == conj(log(z)).

@asmeurer
Copy link
Member

Hmm, OK. What about functions whose branch cuts are along the imaginary axis?

Anyway, let's make sure this is tested properly when we add it to the test suite (specifically, with +/- 0 being tested) @honno

@asmeurer
Copy link
Member

Just to be clear, I'm OK with this PR. My question applies to some other functions, but it's not something that should block things, since it's just about a special case which could easily be added later.

@rgommers
Copy link
Member

Thanks @asmeurer for the clarification and the new issue. Looks like we're all good here then.

@rgommers rgommers merged commit 971c06a into main Dec 13, 2022
@rgommers rgommers deleted the cmplx-log branch December 13, 2022 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants