Guard solve_up Enzyme reverse rule against runtime-activity aliased shadows#3740
Open
SebastianM-C wants to merge 1 commit into
Open
Guard solve_up Enzyme reverse rule against runtime-activity aliased shadows#3740SebastianM-C wants to merge 1 commit into
SebastianM-C wants to merge 1 commit into
Conversation
…hadows Under `set_runtime_activity`, a runtime-inactive value reaches the rule as Duplicated/MixedDuplicated whose shadow IS the primal (dval === val). The reverse rule for `DiffEqBase.solve_up` accumulated every non-Const cotangent into `ptr.dval` unconditionally, so e.g. the du0 cotangent was broadcast-added into the caller's primal u0 whenever the solved problem's u0 aliased an array reachable from a Const argument (the common `setsym_oop`/`remake` pattern in MTK loss functions). The first gradient call returned correct results while silently corrupting the Const problem's u0; subsequent calls were garbage. Skip accumulation when the shadow aliases the primal — a runtime-inactive value accumulates nowhere, exactly as if it were Const. Found while reducing SciML/SciMLSensitivity.jl#1477 (failure mode B). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
Seems reasonable but this should definitely get a test. |
Member
Author
|
Sure, but where would that go? From what I see SciMLSensitivity related testing is done via downstream. Should I PR the test to SciMLSensitivity? |
Member
|
either there or the downstream ad test group |
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
2 participants
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.
While looking into reducing SciML/SciMLSensitivity.jl#1477 with Claude, I found a potential bug in the Enzyme rule under
set_runtime_activity(Reverse)when theu0is aliased inremake(which happens becauseSII.setsym_oopreturns theprob.u0if there are no changes). The fix that Claude proposed was to skip accumulation when the shadow aliases the primal, exactly as if it wereConst.@ChrisRackauckas Would this be appropriate or should we always copy in
SII.setsym_oop(or both)?Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.