search

JuliaDiff / DiffRules.jl

A simple shared suite of common derivative definitions
Other
74 stars 38 forks source link

Revert "comment out trinary rules" #62

Open mcabbott opened 3 years ago

mcabbott commented 3 years ago

This reverts #60, thus un-reverts #54.

Wants to be run with #61 and https://github.com/JuliaDiff/ForwardDiff.jl/pull/530 before merging.

codecov-commenter commented 3 years ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 :tada:

Comparison is base (2001650) 97.86% compared to head (6a7aeef) 97.89%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #62 +/- ## ========================================== + Coverage 97.86% 97.89% +0.03% ========================================== Files 3 3 Lines 187 190 +3 ========================================== + Hits 183 186 +3 Misses 4 4 ``` | [Impacted Files](https://codecov.io/gh/JuliaDiff/DiffRules.jl/pull/62?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/rules.jl](https://codecov.io/gh/JuliaDiff/DiffRules.jl/pull/62?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL3J1bGVzLmps) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mcabbott commented 2 years ago

Thanks for looking. I thought so but indeed we should re-check.

ForwardDiff v0.10.18 and below have the error if there is a trinary rule. This is the line upper-bounding their DiffRules in the registry:

https://github.com/JuliaRegistries/General/blob/master/F/ForwardDiff/Compat.toml#L50-L51

The current version is ForwardDiff v0.10.19 which accepts only DiffRules 1.2.1 and up:

https://github.com/JuliaDiff/ForwardDiff.jl/blob/v0.10.19/Project.toml

This is reflected here in the registry:

https://github.com/JuliaRegistries/General/blob/master/F/ForwardDiff/Compat.toml#L22-L23

lines added by the robot https://github.com/JuliaRegistries/General/pull/41858 . So future releases should also work smoothly I believe, not touch old bounds.

So I think that implies this is safe.

devmotion commented 1 year ago

@mcabbott I updated the PR and fixed some tests, do you have any comments? Otherwise I think this PR can be merged.