-
Notifications
You must be signed in to change notification settings - Fork 6
Autodiff Support and Allocation Cleanup #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autodiff Support and Allocation Cleanup #12
Conversation
…xTransformations.jl into feat-all-autodiff-backends
…xTransformations.jl into feat-all-autodiff-backends
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 30
Lines 1313 1313
=========================================
Hits 1313 1313 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @jmurphy6895 ! Awesome PR! Thanks! One question: can we make the functions verbose by default? I am only trying to avoid changing the default behavior for those who do not want differentiation. |
|
By the way, any idea why the tests are failing? |
|
I'll go through and resolve the failing tests, my guess is it has to do with new versions that got freed by removing them from the compat I'll try resolve by end of week |
I actually added this change to make the function non-allocating. The kwargs there weren't exposed in any of the other functions so I made it false by default, with the thought that anyone modifying that function would have access to it. I can try bubbling it up to more common interfaces via a kwarg if that's desired, when I went and tried it the first time it just ended being in a lot of places. What are your thoughts? Edit: What if it was true by default in the function call itself but in the interfaces it was defaulted to false? I don't think a user can trigger it without working directly with the function |
|
Hum, if it removes allocations, maybe it is better to let it false by default. Since it is just a waring, maybe I can consider it non-breaking. |
|
The tests are failing due to changes to Zygote 0.6 -> 0.7, I address them here JuliaSpace/ReferenceFrameRotations.jl#35. I can add in the gating for certain tests on nightly similar to JuliaSpace/SatelliteToolboxGravityModels.jl#4, if the work-around there is acceptable |
|
Yes, the work around seems perfect! |
|
Thanks! |
Added fixes and tests for a set of autodiff backends and cleaned up some allocations