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

Ignore rules with more than two arguments #530

Merged
merged 3 commits into from
Jul 26, 2021

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Jul 4, 2021

DiffRules.jl supports N-ary rules. These aren't automatically handled by ForwardDiff, but I don't think their existence need cause this package not to be usable.

If the two packages must be coupled more tightly than via Semver, then perhaps a test checking that all rules have Dual definitions would be appropriate. And produce a test failure, while allowing use of other, unaffected, parts of the package.

Closes #529, I think.

Edit to add context:

  • DiffRules v1.1 and v1.2 contain 3-ary rules. These were removed from 1.2.1 as a temporary measure, and the General registry changed so that existing versions of ForwardDiff are marked incompatible with 1.1 and 1.2.
  • The present Project.toml declares compatibility with DiffRules 1.0. I believe that tagging a new release without this PR will mean it is marked compatible with all 1.0 versions, and thus combinations which crash on launch will be legal according to the registry.

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2021

Codecov Report

Merging #530 (ab26d8d) into master (ea822d2) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
+ Coverage   84.72%   84.83%   +0.11%     
==========================================
  Files           9        9              
  Lines         825      831       +6     
==========================================
+ Hits          699      705       +6     
  Misses        126      126              
Impacted Files Coverage Δ
src/dual.jl 72.72% <100.00%> (+0.58%) ⬆️

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 ea822d2...ab26d8d. Read the comment docs.

@mcabbott
Copy link
Member Author

mcabbott commented Jul 4, 2021

56df4e9 skips some tests around test/HessianTest.jl:133. These also failed in #522, indicating that whatever causes them, it wasn't recent changes to DiffRules.

Edit -- now fixed.

@KristofferC
Copy link
Collaborator

The broken test are from a regression in StaticArrays: JuliaArrays/StaticArrays.jl#907 (comment)

This reverts commit b32620e.
@KristofferC KristofferC merged commit 54fe5d0 into JuliaDiff:master Jul 26, 2021
@mcabbott mcabbott deleted the tri_rules branch July 26, 2021 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DiffRules [1.1,1.2] breaks precompilation
3 participants