AD rule for logerfcx

[cossio]

This should fix https://github.com/FluxML/Zygote.jl/issues/1132.

[codecov-commenter]

Codecov Report

Merging #74 (f888ee5) into master (0aa8c72) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            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              

Impacted Files	Coverage Δ
src/rules.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0aa8c72...f888ee5. Read the comment docs.

[devmotion]

Can also fix the doctest error? I assume either the additional definition or Julia 1.7 leads to a differently ordered set of rules.

[cossio]

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 diffrules, which is not reliable. I think it can be fixed by making the order deterministic, https://github.com/JuliaDiff/DiffRules.jl/pull/75.

[mcabbott]

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...

[cossio]

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).

[oxinabox]

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.

Checking isdefined is best, when that is what you care about. And doesn't add too much complexity like in this case.

I think keeping this package at zero dependencies is worth something

This package has always depended on SpecialFunctions.jl (well ever since SpecialFunctions.jl was peeled out of Base at least)

[cossio]

This package has always depended on SpecialFunctions.jl

@oxinabox Yes, but it wasn't imported. To check isdefined I need to import it. Do we want to import it?

Why support Julia 1.0 anyway?

[mcabbott]

has always depended on SpecialFunctions.jl

Well, for some definitions of "depended on ". This package does not load SpecialFunctions, it only has a version bound.

[oxinabox]

@oxinabox Yes, but it wasn't imported. To check isdefined I need to import it.

Fair point.

Counter, AFAIK every package that depends on DiffRules depends on SpecialFunctions.jl also. And does load it. so ::shrug::

[devmotion]

As mentioned in the thread, I don't think updating the Julia compat is a correct approach.

IMO the best solution is to load it. There's no need to update the Julia version bound - it only addresses the problem indirectly and we will want to add more rules that were introduced in SpecialFunctions > 1 for which this hack does not work.

[mcabbott]

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?

[devmotion]

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 import SpecialFunctions would be bad. It doesn't affect the dependencies of DiffRules and eg precompilation already includes direct dependencies and hence SpecialFunctions.

[mcabbott]

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.

[cossio]

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.

If you increase the bound on SpecialFunctions this will also increase the bound on Julia (because SpecialFunctions has a higher lower bound for Julia)

[mcabbott]

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.

[devmotion]

Exactly, it means dropping support for older Julia versions, im- or explicitly. Since we use the same approach (isdefined) already for LogExpFunctions, I think the easiest way would be to also use it in this PR and to not touch compat bounds. It's maybe cleaner to discuss and work towards zero dependencies in a separate issue or PR.

[mcabbott]

Seems fine to drop < 1.3, no?

76 reverts this check, back to zero deps.

[cossio]

What bound exactly do you need on SpecialFunctions?

Need SpecialFunctions >= 0.10, which defined logercfx. In turn, this requires Julia 1.3, https://github.com/JuliaMath/SpecialFunctions.jl/blob/v0.10.0/Project.toml.

[cossio]

Seems fine to drop < 1.3, no?

Ok. But why not jump directly to 1.6, which is the new LTS?

[devmotion]

Seems fine to drop \u003C 1.3, no?

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).

[cossio]

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.

[devmotion]

As I explained I'm against changing any compat bounds in this PR (not in general, but in this PR).

[mcabbott]

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.

I see that CI only runs on 1.3 anyway already, BTW. Edit -- hadn't noticed that this was a change of this PR, not before. On master it runs on 1.0 still.

Maybe worth making another 1-line PR to bump the lowest Julia version to match? That way anyone watching is more likely to notice.

[cossio]

I see that CI only runs on 1.3 anyway already, BTW.

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).

Maybe worth making another 1-line PR to bump the lowest Julia version to match? That way anyone watching is more likely to notice.

Sorry, the lowest version where?

[mcabbott]

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.

[cossio]

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).

[mcabbott]

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.

[cossio]

No, we're suggesting making another one-line PR which will 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.

[cossio]

@mcabbott @devmotion Would appreciate it if this got merged quickly (if we all agree of course). https://github.com/JuliaDiff/DiffRules.jl/pull/77

[cossio]

I had fixed all conflicts already but it seems you force pushed some other changes smile 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! Sure, done.

[cossio]

@mcabbott Do we merge this?

[cossio]

@mcabbott @devmotion Can we tag a release with this :pray: ?

[devmotion]

Done: https://github.com/JuliaRegistries/General/pull/50371

[cossio]

Thanks!