Skip to content

Conversation

@sethaxen
Copy link
Collaborator

Following after #56 (comment), after further thought, implementing cis (and cispi) seems dangerous, as our implementation disagrees with the docstring in Base. A few other relevant notes.

Of all overloads defined in #56, its complex version is the only function that takes a real number to a complex number (i.e. is specific to Complex), and it's the only function that violates f(z') == f(z)'.

Its power series representation is cis(z) = \sum_{n=0}^\infty im^n/n! z^n, so that its coefficients are a_n = im^n/n!.
However, the cis overload we define is actually equivalent to cis2(z) = cis(z * sign(imag(z))), which has the power series representation: cis2(z) = \sum_{n=0}^\infty (sign(imag(z)) im)^n/n! z^n. These are not the same function:

julia> z = 1+3im
1 + 3im

julia> (cis(z), cis2(z), cis(quat(z)))  # all the same
(0.02690006784157161 + 0.041894373450204546im, 0.02690006784157161 + 0.041894373450204546im, QuaternionF64(0.02690006784157161, 0.041894373450204546, 0.0, 0.0, false))

julia> (cis(z'), cis2(z'), cis(quat(z')))  # different
(10.852261914197959 + 16.901396535150095im, 0.02690006784157161 - 0.041894373450204546im, QuaternionF64(0.02690006784157161, -0.041894373450204546, 0.0, 0.0, false))

This PR removes cis/cispi. By the above arguments, implementing it was a bug, and this is a non-breaking change.

@codecov
Copy link

codecov bot commented Feb 26, 2022

Codecov Report

Merging #76 (1083d5b) into master (eebbf5b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #76   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          397       391    -6     
=========================================
- Hits           397       391    -6     
Impacted Files Coverage Δ
src/Quaternion.jl 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hyrodium
Copy link
Collaborator

hyrodium commented Feb 26, 2022

By the above arguments, implementing it was a bug, and this is a non-breaking change.

I think this PR can be considered a breaking change since it removes the feature instead of solving the issue. Since v1.0 has not been released yet, I think it will not be a problem with a breaking change.

@sethaxen
Copy link
Collaborator Author

Sure, we can hold until it for now until we make other breaking changes; it's unlikely anyone will be using cis.

Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Quaternions.jl v0.5.7 has been released, and this PR can be merged after resolving conflicts.

@sethaxen sethaxen merged commit ac2da65 into JuliaGeometry:master Oct 2, 2022
@sethaxen sethaxen deleted the rmcis branch October 2, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants