Fix ternary hypot flattening Duals with different tags - #835
Open
devmotion wants to merge 1 commit into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #835 +/- ##
==========================================
+ Coverage 90.71% 91.49% +0.78%
==========================================
Files 11 11
Lines 1055 1058 +3
==========================================
+ Hits 957 968 +11
+ Misses 98 90 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
All seven bodies of `hypot`'s `@define_ternary_dual_op` delegated to a single
helper that unwrapped every argument with the one-argument `value`/`partials`,
irrespective of its tag. Arguments with an inner tag were therefore flattened
and their partials summed into the tag selected by dispatch, which annihilates
their perturbations and pollutes the surviving tag's partials.
Give each case its own body that only unwraps the arguments known to carry the
tag, as is done for `fma` and `muladd`. Since `hypot` is symmetric, two helpers
plus permutations of their arguments cover all seven cases. Because the tag is
now read off the helpers' signatures instead of being passed as a `::Type{T}`
argument, ternary `hypot` also starts working for non-`Type` tags such as
`Dual{:t}`, which previously threw a `MethodError`.
The tests cover each of the seven bodies twice over: once with the non-tagged
arguments as plain `Real`s and once with them carrying an inner tag.
Fixes #834
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
devmotion
force-pushed
the
dmw/ternary-hypot-tags
branch
from
August 6, 2026 14:01
adcf4a9 to
4600adb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #834.
All seven bodies of the
@define_ternary_dual_opforhypotdelegated to a single helper that unwraps every argument with the one-argumentvalue/partials, regardless of its tag:When the arguments carry different tags, the ones with an inner tag are flattened as well and their partials are summed into whichever tag dispatch selected — their perturbations are annihilated, and the surviving tag's partials are polluted by their contributions:
Each case now gets its own body that only unwraps the arguments known to carry the tag, as
fmaandmuladdalready do. Arguments with an inner tag are passed on unchanged; sinceDual <: Realthey are handled by the recursivehypotcall, which keeps their perturbations nested inside the returnedDual. Becausehypotis symmetric, two helpers plus permutations of their arguments cover all seven cases.Since the tag is now read off the helpers' signatures instead of being passed as a
::Type{T}argument, this also makes ternaryhypotwork for non-Typetags, which previously threw aMethodError:Note that
calc_hypotis not just arity plumbing and cannot simply be dropped in favour of composing binary operations: it calls Base'shypoton the values and applies the analytic∂h/∂xᵢ = xᵢ/h, so no squares are formed and the partials do not overflow. There is a test for this.Unchanged: the values and partials for single-tag arguments (including gradients and Hessians), and the
NaNwhen differentiating at the origin, wherehypotgenuinely has no gradient.nansafe_modedoes not help there because theNaNoriginates in the coefficientvx/h = 0/0rather than in a zero partial, and the two-argumenthypotfrom DiffRules behaves the same way.Tests
Each of the seven bodies is now covered twice over — once with the non-tagged arguments as plain
Reals, in the "Special Cases" block, and once with them carrying an inner tag, in the new testset. Both are needed: dispatch settles on one tag, and a different body runs depending on whether one or two of the arguments carry it, so a nestedderivativewith the differently-tagged argument in only one position reaches just five of the seven bodies.The new tests fail on master in all three argument positions and for three distinct tags (
0.0instead of the correct derivative). Mis-permuting any of the six non-symmetric arguments of the@define_ternary_dual_opcall also turns them into aMethodError, so the permutations are checked rather than merely written down.Full test suite passes locally on Julia 1.12.6 with
nansafe_modedisabled (9279/9279) and enabled (9361/9361).