-
Notifications
You must be signed in to change notification settings - Fork 38
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
AD rule for logerfcx #74
Conversation
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
==========================================
+ Coverage 96.93% 96.95% +0.01%
==========================================
Files 3 3
Lines 163 164 +1
==========================================
+ Hits 158 159 +1
Misses 5 5
Continue to review full report at Codecov.
|
Can also fix the doctest error? I assume either the additional definition or Julia 1.7 leads to a differently ordered set of rules. |
Regarding this, the current jldoctest relies on a specific order of returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, if tests pass.
Shouldn't this just change the version bound, rather than check existence of a method? Cc @oxinabox, who had strong opinions the last time this came up. I think keeping this package at zero dependencies is worth something, although I suspect this is a losing battle and we will all soon depend on a dozen leftpad packages... |
I agree increasing the version bound would be best, both of Julia (1.0 -> 1.6, the new LTS), and SpecialFunctions (minimum should be 0.10, so we drop support for 0.8, 0.9). |
Checking
This package has always depended on SpecialFunctions.jl |
@oxinabox Yes, but it wasn't Why support Julia 1.0 anyway? |
Well, for some definitions of "depended on ". This package does not load SpecialFunctions, it only has a version bound. |
Fair point. Counter, AFAIK every package that depends on DiffRules depends on SpecialFunctions.jl also. |
As mentioned in the thread, I don't think updating the Julia compat is a correct approach.
|
A majority, but by no means all, of the packages using this do load SpecialFunctions: https://juliahub.com/ui/Packages/DiffRules/x2ZNk/1.5.0?page=2 Updating the version bound seems like the smallest change, it's just changing the setting on the existing mechanism. There is no reason this won't work for functions added in version > 1, you just move the bound to 1.2, surely? What am I missing? |
I was referring to the suggestion of bumping the julua compat to avoid this issue. SpecialFunctions added functions post-1.0 for which we want to define rules at some point for which this hack does not work. Hence I'm against changing the Julia compat. I would be fine with bumping eg SpecialFunctions compat entry but it's not clear to me yet why |
Sorry, yes I only mean changing the bound on SpecialFunctions. The bound on Julia itself is another question, and I agree that would be a very indirect way to constrain anything. This package did and (I think) still can have zero dependencies. That's the best situation, and I think we ought to have a solid reason to move away from it. |
If you increase the bound on SpecialFunctions this will also increase the bound on Julia (because SpecialFunctions has a higher lower bound for Julia) |
What bound exactly do you need on SpecialFunctions? No objection to increasing the bound on Julia as a consequence, the confusion is over increasing the bound on Julia as a mechanism to bound SpecialFunctions. |
Exactly, it means dropping support for older Julia versions, im- or explicitly. Since we use the same approach ( |
Seems fine to drop < 1.3, no? #76 reverts this check, back to zero deps. |
Need SpecialFunctions >= 0.10, which defined logercfx. |
Ok. But why not jump directly to 1.6, which is the new LTS? |
I don't have a strong opinion (I think it's fine) but I think updating julia compat bounds deserves a separate PR to make it more transparent for other contributors and hence easier for them to state their opinion on the matter (and eg to discuss if it should be bumped to 1.6). |
Ok. I agree that 1.6 can be discussed elsewhere. But we need to make a decision regarding 1.3. This is directly relevant to this PR, since Julia 1.3 is a necessary dependence of logerfcx. If the scope of this PR is to support logerfcx, then it is completely fine to increase the bound here, I think. |
As I explained I'm against changing any compat bounds in this PR (not in general, but in this PR). |
IMO 1.6 being LTS is a reason to be more comfortable with dropping 1.0, but there is nothing gained by moving the package's lower bound higher than we have reason to, i.e. higher than 1.3.
Maybe worth making another 1-line PR to bump the lowest Julia version to match? That way anyone watching is more likely to notice. |
I took the liberty of adding 1.6 to CI here. But if you prefer we can do that in a separate PR (although I see no reason why).
Sorry, the lowest version where? |
I mean, as David suggest, making an independent PR to change the version bound on Julia in Project.toml. It need do nothing else at all, since CI already does not run on 1.0. |
Are you saying that here I should leave Julia 1.0 as the low compat bound, instead of 1.3? I'm not sure that would work. I mean, if I add the SpecialFunctions >= 0.10 compat bound, and then try to run CI on Julia 1.0, it won't work (because SpecialFunctions 0.10 requirse Julia 1.3). |
No, we're suggesting making another one-line PR which will change the bound. After that one, this one won't have to change the bound. |
Ah I see, you suggest to make that PR before this one. Ok. If you merge it quickly I can make it right now. |
@mcabbott @devmotion Would appreciate it if this got merged quickly (if we all agree of course). |
Zygote uses ForwardDiff for differentiating broadcasting operations. See FluxML/Zygote.jl#1001. However ForwardDiff cannot differentiate non-generic code that accepts only Float64 (it needs to pass its Dual type). In addition ForwardDiff defines rules via DiffRules. It doesn't understand ChainRules. This means we need to define some rules here to make things work. See discussion here to understand how to add new rules for ForwardDiff: https://discourse.julialang.org/t/issue-with-forwarddiff-custom-ad-rule/72886/6 If JuliaDiff/DiffRules.jl#74 gets merged I should not need this anymore.
logerfcx was introduced in SpecialFunctions 0.10, which in turn requires Julia 1.3. Therefore we don't do CI on Julia 1.0, but on 1.3. Also added the LTS, 1.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had fixed all conflicts already but it seems you force pushed some other changes 😄 In particular, I had already bumped the version number to 1.8.0 - can you update the version number again?
Oh I'm sorry, didn't realize this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
@mcabbott Do we merge this? |
@mcabbott @devmotion Can we tag a release with this 🙏 ? |
Thanks! |
This got merged, so we can removed it here. JuliaDiff/DiffRules.jl#74
We need JuliaDiff/DiffRules.jl#74 otherwise we hit FluxML/Zygote.jl#1132 This got merged in DiffRules 1.8. So even if we don't import DiffRules directly, we add as a dep in Project.toml, just to add the compat bound.
This should fix FluxML/Zygote.jl#1132.