Add `realdot` and `imagdot` from ChainRules #474

devmotion · 2021-10-01T15:03:35Z

This PR adds ~~realconjtimes~~realdot and ~~imagconjtimes~~imagdot, currently defined in ChainRules as _realconjtimes and _imagconjtimes. These optimizations of real(conj(x) * y), or more generally real(dot(x, y)), and imag(conj(x) * y)/imag(dot(x, y)) are useful more generally and could be used e.g. in JuliaMath/SpecialFunctions.jl#350.

I extracted src/rulesets/Base/utils.jl (and its history) from ChainRules, renamed the functions, and added some tests and documentation.

* Fix indentation * Test \ on complex inputs * Test ^ on complex inputs * Test identity on complex inputs * Test muladd on complex inputs * Test binary functions on complex inputs * Test functions on complex inputs * Release type constraint on exp * Add _realconjtimes * Use _realconjtimes in abs/abs2 rules * Add complex rule for hypot * Add generic rule for adjoint * Add generic rule for real * Add generic rule for imag * Add complex rule for hypot * Add rules/tests for Complex * Test frule for identity * Add missing angle test * Make inline just in case * Unify abs rules * Introduce _imagconjtimes utility function * Unify angle rules * Unify sign rules * Multiply by correct variable * Fix argument order * Bump ChainRulesTestUtils version number * Restrict to Complex * Use muladd * Update src/rulesets/Base/fastmath_able.jl Co-authored-by: willtebbutt <[email protected]> Co-authored-by: willtebbutt <[email protected]>

* rename DoesNotExist * rename Composite * bump version and compat * rename Zero * remove typos * reexport deprecated types manually

devmotion · 2021-10-01T15:04:12Z

docs/src/api.md

-Pages = ["rule_definition_tools.jl"]
+Pages = [
+    "rule_definition_tools.jl",
+    "utils.jl",


Maybe they should be moved to src/rule_definition_tools.jl?

That file is full of metaprogramming stuff.
It doesn't need this.

Files called utils.jl are always a mistake.
I think something like complex_math.jl as it currently stands,
or additional_mathematical_functions.jl if we think we will need more of them later.

codecov-commenter · 2021-10-01T15:17:41Z

Codecov Report

Merging #474 (9c3cab2) into master (d58e420) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   92.89%   93.00%   +0.11%     
==========================================
  Files          15       16       +1     
  Lines         816      829      +13     
==========================================
+ Hits          758      771      +13     
  Misses         58       58

Impacted Files	Coverage Δ
src/ChainRulesCore.jl	`100.00% <ø> (ø)`
src/utils.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 d58e420...9c3cab2. Read the comment docs.

sethaxen · 2021-10-01T15:35:08Z

It might be worth generalizing this to realdot (or dotr) and imagdot (or doti). That's what's really going on here anyways, and, while less common, something like real(dot(x, y)) or imag(dot(x, y)) appears in various places in this codebase:

For now, realdot(x, y) = real(dot(x, y)) as a fallback would be fine. We might even consider adding a more efficient implementation for arrays in the future.

oxinabox · 2021-10-01T15:37:11Z

I am not sure if this is worth adding to our API surface?
Is it that bad to copy-paste this around to places it is needed?
If we had 3 packages that wanted it, i can see it maybe.
But two, maybe not?

idk I could be convinced that this is a crucial tool needed to define rules for nonholomorphic functions.
but I am not yet convinced

sethaxen · 2021-10-01T15:40:59Z

Oh I didn't notice this was ChainRulesCore. Then yes, in that case, I do not know if this is worth adding here, in the spirit of keeping this package as-simple-as-possible.

devmotion · 2021-10-01T15:57:20Z

I'm not fully convinced either, that's why I asked in the SpecialFunctions PR 😄

I assumed it could be useful since it came up in these simple cases in SpecialFunctions and hence I assumed that it would likely pop up in other derivatives of non-holomorphic functions as well. I came across ChainRules._realconjtimes only by accident, and I assume if it is not part of the API or at least documented more clearly people won't realize that it exists and could be copied.

I still think it would be a bit annoying to have to copy _realconjtimes to SpecialFunctions, to me it seems like an optimization that other packages should not have to think about. Hence I also just used real(conj(x) * y) in the SpecialFunctions PR and neglected these possible optimizations.

… into dw/conjtimes

oxinabox

I am not yet done thinking about this

nickrobinson251 · 2021-10-04T11:28:30Z

i am not sure how much my opinion on this is worth, as i'm unlikely to be a user of the code added, but fwiw...

I think adding these particular methods to CRC seems fine to me, in a complex_math.jl file, so long as we’re going to have a really high bar for adding “helper” functions like this... because we want CRC to be as minimal as reasonably possible, and also i think maintainence-wise we don’t really want CRC to be a place that keeps being added to over time

maybe rough ideas of what “a high-bar” means here:

at least 3 packages need it
it is an annoying amount of code (e.g. 7 methods, not 2) or complexity of code (e.g. it took us a while to work out what the correct behaviour was) to copy/maintain in those packages directly
it makes a significant difference to use these helpers rather than the “naive” defintions (e.g. `real(dot(x, y))) in getting important edge-cases correct, and/or optimal performance in common use-cases.

(But as i say, perhaps as someone not often actually writing rules, this is setting the bar too high for adding more helpers functions)

oxinabox · 2021-10-04T11:41:07Z

I agree with this

maybe rough ideas of what “a high-bar” means here:

at least 3 packages need it

it is an annoying amount of code (e.g. 7 methods, not 2) or complexity of code (e.g. it took us a while to work out what the correct behaviour was) to copy/maintain in those packages directly

it makes a significant difference to use these helpers rather than the “naive” defintions (e.g. `real(dot(x, y))) in getting important edge-cases correct, and/or optimal performance in common use-cases.

I think as of right now now it doesn't quite meet the bar.
In particular it is needed in two packages (ChainRules.jl and SpecialFunctions.jl).
I do think that sooner or later we are likely to need it in another.
But I think we can just sit on this PR til that happens.
(and copy the code over to the packages and leave a link back to this PR in a comment)

devmotion · 2021-10-09T22:26:44Z

Just came across https://github.com/JuliaStats/StatsBase.jl/blob/0a179531f14d0140356fcf1f1d55bd0240030cf2/src/scalarstats.jl#L259-L261, so I found another use case for realdot but it is not related to ChainRules 😂

Maybe the example indicates though that it could be useful to add them to base?

sethaxen · 2021-10-11T15:25:41Z

src/utils.jl

+@inline realdot(x::Real, y::Complex) = x * real(y)
+@inline realdot(x::Complex, y::Real) = real(x) * y
+@inline realdot(x::Real, y::Real) = x * y


Should be safe to generalize the cases, and then the last one is redundant since real(::Real) is a no-op

Suggested change

@inline realdot(x::Real, y::Complex) = x * real(y)

@inline realdot(x::Complex, y::Real) = real(x) * y

@inline realdot(x::Real, y::Real) = x * y

@inline realdot(x::Real, y::Number) = x * real(y)

@inline realdot(x::Number, y::Real) = real(x) * y

Won't this introduce an ambiguity, when both are real?

Yes, that's right.

src/utils.jl

sethaxen · 2021-10-11T15:32:55Z

I don't think these have been shown to be useful enough to be in base Julia, but it might be worth putting them in their own ultra-lightweight package. Then we can point to that package in our documentation, and package developers can depend on it if they find it useful. Simultaneously resolves the concerns with putting it here while still making the functionality available.

Co-authored-by: Seth Axen <[email protected]>

devmotion · 2021-10-11T18:57:32Z

The only problem with moving them to a separate package seems to be that imagdot returns a ZeroTangent when the output of dot is guaranteed to be a real number. This would only be possible if the package depends on ChainRulesCore. Of course, this is possible but limits its use to AD computations for CRC. Maybe this optimization/specialization should be dropped and it should just return false?

sethaxen · 2021-10-11T20:44:51Z

The only problem with moving them to a separate package seems to be that imagdot returns a ZeroTangent when the output of dot is guaranteed to be a real number. This would only be possible if the package depends on ChainRulesCore.

Yeah, that's right. I'm thinking both imagdot and realdot could go in such a package, but ChainRules would only use realdot. imagdot is less useful anyways. I don't recall if your PR had a use for it, but in ChainRules that construction only shows up when projecting to be tangent to the complex circle (e.g. angle, sign, logabsdet, and eigen) at 1 + 0im, i.e. dot(x, y) - real(dot(x, y)), and I haven't it seen it come up anywhere else, whereas realdot shows up everywhere.

Of course, this is possible but limits its use to AD computations for CRC. Maybe this optimization/specialization should be dropped and it should just return false?

While false is a strong zero, it's not as strong as Zero(), so the complex branches that use imagdot won't be optimized out when the inputs are real.

devmotion · 2021-10-16T20:30:08Z

I put together a package and added only realdot for now since it seems more generally useful and its implementation does not use ChainRules types (and I'm not sure anymore if a strong zero is really a good idea here, in non-AD scenarios probably one still wants that isnan(imagdot(1im, 1im) * NaN)): JuliaMath/RealDot.jl#2

oxinabox · 2021-10-16T20:49:06Z

Cool, we can add that as a dep for ChainRules.jl and SpecialFunctions.jl?

devmotion · 2021-10-16T22:05:53Z

It's not registered yet (and I'd like @sethaxen to approve the linked PR) but that's the plan 🙂

devmotion · 2021-10-19T14:34:09Z

I initiated the registration process: JuliaRegistries/General#47024

devmotion · 2021-10-24T10:14:20Z

Closed by JuliaDiff/ChainRules.jl#542.

sethaxen and others added 7 commits June 30, 2020 08:00

rename differentials (#413)

4b08ee2

* rename DoesNotExist * rename Composite * bump version and compat * rename Zero * remove typos * reexport deprecated types manually

Add src/rulesets/Base/utils.jl from ChainRules.jl

ace93fb

Rename to realconjtimes and imagconjtimes and export them

0d2269e

Add tests

b1a89ec

Add documentation

3d05206

Bump version

d6c6c2a

devmotion commented Oct 1, 2021

View reviewed changes

Fix tests with Julia 1.0

d9d046f

devmotion mentioned this pull request Oct 1, 2021

Add derivatives for besselix, besseljx, and besselyx JuliaMath/SpecialFunctions.jl#350

Merged

Rename to realdot and imagdot

1747c42

devmotion changed the title ~~Add realconjtimes and imagconjtimes from ChainRules~~ Add realdot and imagdot from ChainRules Oct 1, 2021

devmotion and others added 3 commits October 1, 2021 23:27

Merge branch 'master' into dw/conjtimes

630355e

Add dispatch for real arrays

ff5e71e

Merge branch 'dw/conjtimes' of github.com:JuliaDiff/ChainRulesCore.jl…

9c3cab2

… into dw/conjtimes

oxinabox reviewed Oct 4, 2021

View reviewed changes

oxinabox added the pending-clear-need We are not certain we need this. So waiting for evidence to be presented label Oct 4, 2021

sethaxen reviewed Oct 11, 2021

View reviewed changes

Update src/utils.jl

5155c75

Co-authored-by: Seth Axen <[email protected]>

Generalize ::Complex to ::Number

5fb7ff2

Rename utils.jl to complex_math.jl

a042dd0

devmotion mentioned this pull request Oct 16, 2021

Add realdot JuliaMath/RealDot.jl#2

Merged

devmotion mentioned this pull request Oct 24, 2021

Use RealDot.realdot JuliaDiff/ChainRules.jl#542

Merged

devmotion closed this Oct 24, 2021

devmotion deleted the dw/conjtimes branch October 24, 2021 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `realdot` and `imagdot` from ChainRules #474

Add `realdot` and `imagdot` from ChainRules #474

devmotion commented Oct 1, 2021 •

edited

Loading

devmotion Oct 1, 2021

oxinabox Oct 4, 2021

codecov-commenter commented Oct 1, 2021 •

edited

Loading

sethaxen commented Oct 1, 2021

oxinabox commented Oct 1, 2021

sethaxen commented Oct 1, 2021

devmotion commented Oct 1, 2021

oxinabox left a comment

nickrobinson251 commented Oct 4, 2021 •

edited

Loading

oxinabox commented Oct 4, 2021 •

edited

Loading

devmotion commented Oct 9, 2021

sethaxen Oct 11, 2021

mcabbott Oct 11, 2021

sethaxen Oct 11, 2021

sethaxen commented Oct 11, 2021

devmotion commented Oct 11, 2021

sethaxen commented Oct 11, 2021

devmotion commented Oct 16, 2021

oxinabox commented Oct 16, 2021

devmotion commented Oct 16, 2021

devmotion commented Oct 19, 2021

devmotion commented Oct 24, 2021

Add realdot and imagdot from ChainRules #474

Add realdot and imagdot from ChainRules #474

Conversation

devmotion commented Oct 1, 2021 • edited Loading

devmotion Oct 1, 2021

Choose a reason for hiding this comment

oxinabox Oct 4, 2021

Choose a reason for hiding this comment

codecov-commenter commented Oct 1, 2021 • edited Loading

Codecov Report

sethaxen commented Oct 1, 2021

oxinabox commented Oct 1, 2021

sethaxen commented Oct 1, 2021

devmotion commented Oct 1, 2021

oxinabox left a comment

Choose a reason for hiding this comment

nickrobinson251 commented Oct 4, 2021 • edited Loading

oxinabox commented Oct 4, 2021 • edited Loading

devmotion commented Oct 9, 2021

sethaxen Oct 11, 2021

Choose a reason for hiding this comment

mcabbott Oct 11, 2021

Choose a reason for hiding this comment

sethaxen Oct 11, 2021

Choose a reason for hiding this comment

sethaxen commented Oct 11, 2021

devmotion commented Oct 11, 2021

sethaxen commented Oct 11, 2021

devmotion commented Oct 16, 2021

oxinabox commented Oct 16, 2021

devmotion commented Oct 16, 2021

devmotion commented Oct 19, 2021

devmotion commented Oct 24, 2021

Add `realdot` and `imagdot` from ChainRules #474

Add `realdot` and `imagdot` from ChainRules #474

devmotion commented Oct 1, 2021 •

edited

Loading

codecov-commenter commented Oct 1, 2021 •

edited

Loading

nickrobinson251 commented Oct 4, 2021 •

edited

Loading

oxinabox commented Oct 4, 2021 •

edited

Loading