Partial implementation of Zygote AD with symbolic indexing #479
Conversation
Symbolic indexing of state and observed variables. Time index must be specified or observed.
![ai-maintainer[bot]](https://avatars.githubusercontent.com/in/339956?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI-Maintainer Review for PR - Partial implementation of Zygote AD with symbolic indexing
Title and Description 👍
Scope of Changes 👍
Testing ⚠️
Documentation ⚠️
ZygoteExt.getindex(VA::ODESolution, i::Int, j::Int)ZygoteExt.getindex(VA::ODESolution, sym, j::Int)ChainRulesCore.rrule(config::ChainRulesCore.RuleConfig, ::typeof(getindex), VA::ODESolution, sym, j::Integer)ChainRulesCore.rrule(::typeof(getindex), VA::ODESolution, sym)ZygoteExt.getindex(VA::ODESolution, i::Int)ZygoteExt.getindex(VA::ODESolution, sym)
These functions, classes, or methods should have docstrings added to describe their behavior, arguments, and return values.
Suggested Changes
- Please add docstrings to the new functions, classes, or methods to describe their behavior, arguments, and return values.
- Please provide information about how the changes were tested. This could include specific test cases or scenarios that were used to verify the correctness and functionality of the implemented symbolic indexing.
Reviewed with AI Maintainer
Codecov Report
@@ Coverage Diff @@
## master #479 +/- ##
==========================================
+ Coverage 53.73% 56.82% +3.08%
==========================================
Files 47 50 +3
Lines 3584 3657 +73
==========================================
+ Hits 1926 2078 +152
+ Misses 1658 1579 -79
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ext/ZygoteExt.jl
Outdated
| Δ′ = ODESolution{ | ||
| T, | ||
| N, | ||
| typeof(du), | ||
| Nothing, | ||
| Nothing, | ||
| typeof(VA.t), | ||
| typeof(VA.k), | ||
| typeof(dprob), | ||
| typeof(VA.alg), | ||
| typeof(VA.interp), | ||
| typeof(VA.destats), | ||
| typeof(VA.alg_choice), | ||
| }(du, | ||
| nothing, | ||
| nothing, | ||
| VA.t, | ||
| VA.k, | ||
| dprob, | ||
| VA.alg, | ||
| VA.interp, | ||
| VA.dense, | ||
| 0, | ||
| VA.destats, | ||
| VA.alg_choice, | ||
| VA.retcode) | ||
| (Δ′, nothing, nothing) |
There was a problem hiding this comment.
This is the wrong formatting.
There was a problem hiding this comment.
It won't revert this if you run the formatter, so put it all to one line and then reformat with the right format.
There was a problem hiding this comment.
Changed, maybe fixed?
|
Alright cool this looks good to go if tests pass. I hope we can get |
|
Oh, it fails on 1.6 since package extensions don't exist yet. Should I skip the tests on 1.6 or is there a workaround to get the extensions to load? |
|
Just skip v1.6. We're getting close to dropping it (please LTS come soon). |
Symbolic indexing of state and observed variables. Time index must be specified for observed symbols.
This is a rebase and fixup of https://github.com/lamorton/scimlbase.jl/tree/zygote_rules