Add Aqua tests #775
Add Aqua tests #775
Conversation
Pull Request Test Coverage Report for Build 13840255339Details
💛 - Coveralls |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #775 +/- ##
==========================================
- Coverage 84.56% 84.40% -0.17%
==========================================
Files 34 34
Lines 3830 3840 +10
==========================================
+ Hits 3239 3241 +2
- Misses 591 599 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
penelopeysm
force-pushed
the
py/aqua
branch
3 times, most recently
from
bcb52e0 to
1092558
Compare
penelopeysm
force-pushed
the
py/aqua
branch
2 times, most recently
from
d648df0 to
7146c32
Compare
| # These two StaticTransformation methods needed to resolve ambiguities | ||
| function link!!( | ||
| t::StaticTransformation{<:Bijectors.Transform}, vi::ThreadSafeVarInfo, model::Model | ||
| ) | ||
| return Accessors.@set vi.varinfo = link!!(t, vi.varinfo, model) | ||
| end | ||
|
|
||
| function invlink!!( | ||
| t::StaticTransformation{<:Bijectors.Transform}, vi::ThreadSafeVarInfo, model::Model | ||
| ) | ||
| return Accessors.@set vi.varinfo = invlink!!(t, vi.varinfo, model) | ||
| end |
There was a problem hiding this comment.
Most of the method ambiguity fixes in this PR are really straightforward, but it took me a long time to figure out what to do here.
Firstly, note that this method is only ever invoked in a very specific situation, where we have (1) multithreaded evaluation hence ThreadSafeVarInfo, (2) a StaticTransformation, and (3) the nested varinfo is e.g. a SimpleVarInfo and specifically not a VarInfo, because the ThreadSafeVarInfo{VarInfo} methods are overridden in varinfo.jl.
The DynamicPPL test suite does not ever call this method (if it did, it would have failed with a method ambiguity), and since this has never been reported before, one might justifiably assume that no user has ever called it too.
To be fully honest, while I can't see any reason why this would be wrong, I also can't convince myself 100% that this is the correct behaviour. However, looking at the lines directly above this, it seems clear enough to me that the intent of that code was to suggest that all AbstractTransformations should behave that way, except that DynamicTransformation was to be special-cased. So I figured that it would suffice to duplicate the general AbstractTransformation implementation here.
|
@mhauru I'm still investigating the Aqua failure on 1.10, but I think the code changes can be reviewed. |
|
@mhauru The benchmarks are failing because of a numerically inaccurate Cholesky link/invlink. Fun times TuringLang/Bijectors.jl#356 These should resolve it, but I need to get them merged, and the latter is failing because of something in ChainRules ... which is another reason why I need to learn the AD stuff properly |
|
We need to find a seed that doesn't fail 😅 |
|
(But the tl;dr is that inv/linking LKJCholesky is numerically quite unstable, even for what might be considered 'sensible' inputs. If you're really curious it's explained in those PRs) |
Benchmark Report for Commit
|
|
@mhauru I fixed the benchmarks by adding more StableRNGs. As for the failing tests, the error message points to a Julia 1.10.0 bug (our CI is being run on 'min', which resolves to the lowest possible version). There must be some bug on there that does not let Aqua run properly. When I test locally with 1.10.9, it runs just fine. (Edit: actually when I test locally with 1.10.0 it runs fine too, so the bug is presumably restricted to the CI runner, maybe Linux) So there are a couple of options:
Of these options, I think I prefer (2). What say you? |
This PR adds Aqua tests and fixes all the issues flagged by them. Not much else to describe about it, except for one comment on
ThreadSafeVarInfowhich I put below.