JuliaMath / RealDot.jl

Compute `real(dot(x, y))` efficiently.

MIT License

4 stars 3 forks source link

Add `realdot` #2

Closed devmotion closed 2 years ago

devmotion commented 2 years ago

This PR adds realdot from https://github.com/JuliaDiff/ChainRulesCore.jl/pull/474, which was defined initially (and still is defined) as _realconjtimes in ChainRules.jl.

The commits are a bit messy since I wanted to preserve the git history to give credit to the original authors (mainly @sethaxen) more explicitly. I only added realdot (for now) since it seems more generally useful than imagdot and its implementation did not use any ChainRules-specific types (https://github.com/JuliaDiff/ChainRulesCore.jl/pull/474#issuecomment-940428445).

cc: @sethaxen

codecov[bot] commented 2 years ago

Codecov Report

Merging #2 (c6d1099) into main (ebc598d) will increase coverage by 100.00%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           main        #2        +/-   ##
===========================================
+ Coverage      0   100.00%   +100.00%     
===========================================
  Files         0         1         +1     
  Lines         0         5         +5     
===========================================
+ Hits          0         5         +5

Impacted Files	Coverage Δ
src/RealDot.jl	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ebc598d...c6d1099. Read the comment docs.

devmotion commented 2 years ago

Thanks, I'll merge once tests pass!

One remaining question to consider though: @sethaxen and @oxinabox, do you think I should move it JuliaMath or JuliaDiff before registering it? This wouldn't affect maintenance or have any immediate impact on the package but would mainly make it look a bit more official. I guess this could be helpful if we want to add it as a dependency to ChainRules and SpecialFunctions.

oxinabox commented 2 years ago

Yes, you should move it. It's not really a AD thing as we discovered. So it must be a linear algebra thing. So I guess not JuliaDiff

sethaxen commented 2 years ago

Can you add a test for a number for which only *,conj, adjoint, and real are defined? as a proxy for hypercomplex numbers like Quaternions.Quaternion.

devmotion commented 2 years ago

So it must be a linear algebra thing.

Do you mean the package should be moved to JuliaLinearAlgebra? Since so far it mainly deals with Numbers, I would rather move it to JuliaMath.

devmotion commented 2 years ago

@sethaxen I added a test with quaternions.