Fix #714: Make TAGCOUNT always greater than the last instantiated Tag's count#724
Fix #714: Make TAGCOUNT always greater than the last instantiated Tag's count#724Technici4n wants to merge 2 commits intoJuliaDiff:masterfrom
Conversation
…iated Tag's count
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #724 +/- ##
=======================================
Coverage 90.75% 90.76%
=======================================
Files 11 11
Lines 1071 1072 +1
=======================================
+ Hits 972 973 +1
Misses 99 99 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
devmotion
left a comment
There was a problem hiding this comment.
Is there any way to opt out of precompilation? The design of tagcount is really that no method must exist at the start of a Julia session and tagcount is just defined when it is called with a specific F and V type for the first time.
|
Tested locally on master OrdinaryDiffEq.jl + this branch (with our local widen fix reverted): it does not fix SciML/OrdinaryDiffEq.jl#3381, the discourse MWE, or SciML/NonlinearSolve.jl#932. All three still throw the same The Still a useful fix for #714, just not a structural fix for the SciML-side nested-Dual bugs. |
|
Summary of that is, 3 different downstream MWEs, none of them fixed by this. I don't think this is a real fix. |
If you do that, you can never precompile any ForwardDiff calls 😅 . At least not with tags you will end up using, so then precompilation is effectively ignored by ForwardDiff. That is a fix but it's not really what you want of course. SciML/OrdinaryDiffEq.jl#3587 is super messy though, so I'm looking for if there's a better way to fix it downstream but I don't see one. |
|
Yeah I wrote this a year ago but it is indeed not a great fix. Not sure what can be done to make the tags play well with precompilation, but solving this problem would be huge for the ForwardDiff UX 😅 |
Only precompilation of |
|
Hmmm, not so sure. The tag count is used to nest duals in the right order. If you don't precompile the tag count you also can't precompile anything that nests duals. (Which includes any function you might want to differentiate) |
|
I see. Yes we want to precompile the functions with the tag in there, but we don't care about precompiling the tag count call. |
|
If I remember correctly I was able to fix the bug with this with When I opened this issue #801 to understand the cause for this SciML/OrdinaryDiffEq.jl#3381 , Not sure if this still hold correctness. |
|
I'm missing any intuition and motivation for such a change. Why should we perform |
This fixes #714:
This adds an atomic operation for each
Taginstantiation. This might be problematic when computing many fast derivatives in a tight loop?