JuliaGeometry / Quaternions.jl

A Julia implementation of quaternions
https://juliageometry.github.io/Quaternions.jl
MIT License
116 stars 37 forks source link

Remove cis/cispi #76

Closed sethaxen closed 1 year ago

sethaxen commented 2 years ago

Following after https://github.com/JuliaGeometry/Quaternions.jl/pull/56#issuecomment-1046273555, 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[bot] commented 2 years ago

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% <ø> (ø)

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

hyrodium commented 2 years ago

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 commented 2 years ago

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