search

dpsanders / ReversePropagation.jl

MIT License
52 stars 6 forks source link

register rev function #66

Closed lucaferranti closed 2 years ago

lucaferranti commented 2 years ago

fixes #64

codecov-commenter commented 2 years ago

Codecov Report

Merging #66 (10f67e9) into master (b537354) will decrease coverage by 0.15%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   55.96%   55.80%   -0.16%     
==========================================
  Files           7        7              
  Lines         218      224       +6     
==========================================
+ Hits          122      125       +3     
- Misses         96       99       +3
Impacted Files Coverage Δ
src/ReversePropagation.jl 100.00% <ø> (ø)
src/reverse_icp.jl 95.00% <100.00%> (ø)
src/chain_rules_interface.jl 21.42% <0.00%> (-7.15%) :arrow_down:
src/cse.jl 0.00% <0.00%> (ø)
src/reverse_diff.jl 48.95% <0.00%> (+0.04%) :arrow_up:
src/cse_new.jl 82.14% <0.00%> (+4.36%) :arrow_up:

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 027899c...10f67e9. Read the comment docs.

lucaferranti commented 2 years ago

mmmm 


julia> using ReversePropagation
[ Info: Precompiling ReversePropagation [527681c1-8309-4d3f-8790-caf822a419ba]
WARNING: Method definition promote_symtype(typeof(ReversePropagation.rev), Any...) in module ReversePropagation at C:\Users\lucaa\.julia\packages\Symbolics\WcP64\src\register.jl:82 overwritten on the same line (check for duplicate calls to `include`).
  ** incremental compilation may be fatally broken for this module **
dpsanders commented 2 years ago

I don't see that error with the latest versions of all the dependencies, in a fresh environment and with Julia 1.7.1.

dpsanders commented 2 years ago

I bumped the versions and made ReversePropagation 0.2. Could you try this out please @lucaferranti ?

dpsanders commented 2 years ago

There are some deprecation warnings Warning: Variable{T}(name, idx...) is deprecated, use variable(name, idx...; T=T) though, with Symbolics.jl v4. Would you have a chance to fix those?

lucaferranti commented 2 years ago

yes I'll check in the upcoming days

dpsanders commented 2 years ago

@lucaferranti : It looks like this is ready. Once it's green I'm planning to merge and release the new version

lucaferranti commented 2 years ago

great, thanks! Haven't had the time to do upstream tests with ICP.jl, but if it works here I guess it's fine to merge it now and debug possible further issues in ICP later

dpsanders commented 2 years ago

Many thanks @lucaferranti !