Skip to content
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

add ForwardDiff and ChainRules extension #29

Merged
merged 17 commits into from
Jun 8, 2024
Merged

Conversation

longemen3000
Copy link
Contributor

addresses #28, eq 18 of https://mathworld.wolfram.com/Polylogarithm.html , only real numbers

In theory, the same relation can be done to support the complex domain, but ForwardDiff is a little bit tricky with complex numbers. at least the skeleton (extension loading) is done.

this package supports Julia versions >=1.0, but extensions are only supported in Julia >= 1.9. that means that AD support is only available to new Julia versions. (one alternative would be to add Requires.jl, but it is more cumbersome)

The only thing missing is ChainRules test. (that could be done by loading Zygote, for example, or using ChainRulesTestUtils)

@Expander Expander self-requested a review June 2, 2024 09:58
@Expander
Copy link
Owner

Expander commented Jun 2, 2024

Thanks for this PR!

When I

julia --project test/runtests.jl

I get the following error:

Test Summary: | Pass  Total  Time
digamma       |   11     11  0.1s
Test Summary: | Pass  Total  Time
eta           |   28     28  0.3s
Test Summary: | Pass  Total  Time
fac           |   19     19  0.1s
Test Summary: | Pass  Total  Time
inv_fac       |   20     20  0.1s
Test Summary: | Pass  Total  Time
harmonic      |   19     19  0.2s
Test Summary: |  Pass  Total  Time
li0           | 41267  41267  2.8s
Test Summary: |  Pass  Total  Time
li1           | 41397  41397  2.3s
li2: Error During Test at /home/avoigt/research/PolyLog.jl/test/Li2.jl:71
  Test threw exception
  Expression: ForwardDiff.derivative(PolyLog.reli2, float(pi)) == reli1(float(pi)) / float(pi)
  UndefVarError: `reli1` not defined
  Stacktrace:
   [1] macro expansion
     @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:669 [inlined]
   [2] macro expansion
     @ ~/research/PolyLog.jl/test/Li2.jl:71 [inlined]
   [3] macro expansion
     @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
   [4] top-level scope
     @ ~/research/PolyLog.jl/test/Li2.jl:2
Test Summary: |   Pass  Error   Total   Time
li2           | 102820      1  102821  22.2s
ERROR: LoadError: Some tests did not pass: 102820 passed, 0 failed, 1 errored, 0 broken.
in expression starting at /home/avoigt/research/PolyLog.jl/test/Li2.jl:1
in expression starting at /home/avoigt/research/PolyLog.jl/test/runtests.jl:14

@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Sorry, I still get the same error. I think you should replace ForwardDiff.reli by PolyLog.reli etc.

@longemen3000
Copy link
Contributor Author

Sorry, I still get the same error. I think you should replace ForwardDiff.reli by PolyLog.reli etc.

uff, good catch!

@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Thanks for fixing this!

However, I still get another error:

Test Summary: |  Pass  Total  Time
li6           | 69334  69334  0.8s
li: Error During Test at /home/avoigt/research/PolyLog.jl/test/Li.jl:133
  Test threw exception
  Expression: ForwardDiff.derivative(Base.Fix1(PolyLog.reli, 6), float(pi)) == PolyLog.reli(5, float(pi)) / float(pi)
  MethodError: no method matching reli(::Float64)
  
  Closest candidates are:
    reli(::Integer, ::ForwardDiff.Dual{T}) where T
     @ PolyLogForwardDiffExt ~/research/PolyLog.jl/ext/PolyLogForwardDiffExt.jl:34
    reli(::Integer, ::Real)
     @ PolyLog ~/research/PolyLog.jl/src/Li.jl:26
  
  Stacktrace:
   [1] reli(n::Int64, d::ForwardDiff.Dual{ForwardDiff.Tag{Base.Fix1{typeof(PolyLog.reli), Int64}, Float64}, Float64, 1})
     @ PolyLogForwardDiffExt ~/research/PolyLog.jl/ext/PolyLogForwardDiffExt.jl:36
   [2] (::Base.Fix1{typeof(PolyLog.reli), Int64})(y::ForwardDiff.Dual{ForwardDiff.Tag{Base.Fix1{typeof(PolyLog.reli), Int64}, Float64}, Float64, 1})
     @ Base ./operators.jl:1118
   [3] derivative(f::Base.Fix1{typeof(PolyLog.reli), Int64}, x::Float64)
     @ ForwardDiff ~/.julia/packages/ForwardDiff/PcZ48/src/derivative.jl:14
   [4] macro expansion
     @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:669 [inlined]
   [5] macro expansion
     @ ~/research/PolyLog.jl/test/Li.jl:133 [inlined]
   [6] macro expansion
     @ ~/.julia/juliaup/julia-1.10.3+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
   [7] top-level scope
     @ ~/research/PolyLog.jl/test/Li.jl:7
Test Summary: |    Pass  Error    Total     Time
li            | 3038838      1  3038839  3m15.3s
ERROR: LoadError: Some tests did not pass: 3038838 passed, 0 failed, 1 errored, 0 broken.

To fix it, it would be best to extend the test to "arbitrary" n (i.e. not only $n=6$) by moving the test into the for loop for ni in nis and test reli(n,x) against reli(n-1,x)/x.

@Expander Expander added the enhancement New feature or request label Jun 3, 2024
@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Thanks! Please also move the comment #ForwardDiff Test to the loop. :)

@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Sorry to be pedantic, but could you please also add a test for the derivative of $Li_n(x)$ at $x=0$? If I'm not mistaken, we should have $Li_{n}'(0) = 1$.

@longemen3000
Copy link
Contributor Author

longemen3000 commented Jun 3, 2024

hmm, good insight, the current definitions don't account for that. but it is just the matter of a branch in the derivative definition. at least that holds for reli1 without additional work.

@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Could you please merge the main branch into your branch that you're using for this PR? (It contains a change so we can run all github actions, including the build and test.)

@longemen3000
Copy link
Contributor Author

Is it possible to move the minimum Julia version to 1.6? (the last long term support release)

@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Is it possible to move the minimum Julia version to 1.6? (the last long term support release)

I guess you're asking because of the failing test for julia 1.0?

Just for my understanding: It would require all dependents to also upgrade to 1.6 right? And it would require to move to version 3.0.0, I guess. We would also give up navite support for Debian's oldstable (bullseye).

@longemen3000
Copy link
Contributor Author

Ohh, wait, I know why this is failing, one moment

@Expander Expander linked an issue Jun 3, 2024 that may be closed by this pull request
test/Li.jl Show resolved Hide resolved
@Expander
Copy link
Owner

Expander commented Jun 3, 2024

Great, now all tests pass!

I've noticed that coveralls shows some code that has not been tested, see attached plot. Would you mind checking if it is covered?
coveralls

@Expander
Copy link
Owner

Expander commented Jun 4, 2024

Thanks for fixing this!
Are these if isdefined(Base, :get_extension) statements still necessary for the compatibility with julia 1.0?

@longemen3000
Copy link
Contributor Author

Yeah, the ForwardDiff support is only available after 1.9.
You could support this in 1.0, but that would require to add additional dependencies (Requires.jl)

@Expander
Copy link
Owner

Expander commented Jun 4, 2024

Ok, thanks for the answer!

@Expander
Copy link
Owner

Expander commented Jun 4, 2024

I think so far the chain rules are untested, right? You mentioned that this would be possible with the ChainRulesTestUtils. Would it be possible to add such tests?

test/Li.jl Outdated Show resolved Hide resolved
@Expander
Copy link
Owner

Expander commented Jun 6, 2024

First of all, thanks for adding the tests!

One last thing: I think we also need appropriate entries in the [compat] section for ForwardDiff and ChainRulesCore, right?. Would you mind adding these? (I think it would be best to have a [compat] entry that allows for the most wide range of compatible versions.)

@Expander
Copy link
Owner

Expander commented Jun 7, 2024

I currently get the following error:

┌ Warning: Could not use exact versions of packages in manifest, re-resolving
└ @ Pkg.Operations /opt/hostedtoolcache/julia/1.10.4/x64/share/julia/stdlib/v1.10/Pkg/src/Operations.jl:1814
ERROR: LoadError: Unsatisfiable requirements detected for package ForwardDiff [f6369f11]:
 ForwardDiff [f6369f11] log:
 ├─possible versions are: 0.9.0-0.10.36 or uninstalled
 ├─restricted to versions 0.11 by PolyLog [85e3b03c], leaving only versions: uninstalled
 │ └─PolyLog [85e3b03c] log:
 │   ├─possible versions are: 2.4.2 or uninstalled
 │   └─PolyLog [85e3b03c] is fixed to version 2.4.2
 └─restricted to versions 0.11 by an explicit requirement — no versions left

@Expander
Copy link
Owner

Expander commented Jun 8, 2024

Excellent, thanks a lot! If it is ok with you, I'll merge this PR.

Are you ok with that I add you to a list of contributors?

One final question: Would you mind telling me when version 1.0 of ForwardDiff is out?

@longemen3000
Copy link
Contributor Author

Thanks!, I don't mind, do want you feel is best

On ForwardDiff, No idea, it has been years

@Expander Expander merged commit 281a6bf into Expander:main Jun 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ForwardDiff.jl support?
2 participants