make it work with ForwardDiff v1 #614
Conversation
ajwheeler
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #614 +/- ##
==========================================
+ Coverage 87.07% 87.74% +0.66%
==========================================
Files 28 28
Lines 1888 1811 -77
==========================================
- Hits 1644 1589 -55
+ Misses 244 222 -22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It turns out that it's possible to keep the julia lower bound at 1.6. There are, however, failing tests for 1.6 than for 1.11, so it may still be worth bumping the compatibility requirement. It's just not required to fix compatibility with forward diff. |
mkitti
left a comment
mkitti
left a comment
There was a problem hiding this comment.
The main thing I need to clarify here is the dependency on ForwardDiff. It seems to be both a regular (strong?) dependency and a weak dependency. If this is about compatibility with 1.6, I would prefer to drop 1.6 support in favor of 1.9 with extensions.
I would normally prefer formatting changes to be made in independent pull requests, but I think the amount of such changes in tolerable at the moment.
Project.toml
Outdated
| Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e" | ||
| AxisAlgorithms = "13072b0f-2c55-5437-9ae7-d433b7a33950" | ||
| ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4" | ||
| ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210" |
There was a problem hiding this comment.
Why is ForwardDiff both a strong dep and a weak dep?
There was a problem hiding this comment.
This was in error. Fixed, and restricted Julia version to >= 1.9.
| A2 = rand(Float64, nx, nx) * 100 | ||
|
|
||
| # random array and vector to store gradient | ||
| A2 = rand(Float64, 3, 3) * 100 |
There was a problem hiding this comment.
Why is this hard coded at 3 now?
There was a problem hiding this comment.
Just to make the test less noisy if it fails. There's no particular reason that it had to be nx and I don't there there are any conditions that are hit with 10 points that aren't with 3. That said, I'm happy to change back. It is not an important change.
2 participants
This works, but it's not ready to merge because a decision needs to be made RE. See my comment in #611.