Skip to content

Conversation

@mzgubic
Copy link
Member

@mzgubic mzgubic commented Feb 15, 2022

Closes #241 and #221

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #243 (1bfef83) into main (dd0e246) will increase coverage by 0.39%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
+ Coverage   90.75%   91.15%   +0.39%     
==========================================
  Files          11       12       +1     
  Lines         303      339      +36     
==========================================
+ Hits          275      309      +34     
- Misses         28       30       +2     
Impacted Files Coverage Δ
src/ChainRulesTestUtils.jl 100.00% <ø> (ø)
src/rand_tangent.jl 96.29% <85.71%> (-3.71%) ⬇️
src/global_checks.jl 92.00% <92.00%> (ø)
src/finite_difference_calls.jl 100.00% <100.00%> (+2.77%) ⬆️
src/rule_config.jl 69.23% <100.00%> (+58.11%) ⬆️

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 487890f...1bfef83. Read the comment docs.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this try and use a rrule/frule first then only fallback to using the FiniteDifferences if that returns nothing?

@mzgubic mzgubic changed the title [draft] ADviaFDConfig Implement ADviaFDConfig Feb 17, 2022
@mzgubic
Copy link
Member Author

mzgubic commented Feb 17, 2022

should this try and use a rrule/frule first then only fallback to using the FiniteDifferences if that returns nothing?

Do we ever want to avoid using the rule and make sure to use FD for the inner rule? I don't think so.

I think this means we can just combine the ADviaRuleConfig and ADviaFDConfig into a TestConfig, which does what you describe? And it's not a breaking change because it's not exported?

@oxinabox
Copy link
Member

I think this means we can just combine the ADviaRuleConfig and ADviaFDConfig into a TestConfig, which does what you describe?

Yeah, I guess so.

And it's not a breaking change because it's not exported?

It is, because people use it directly. Don't they?
But we can @deprecate_binding ADviaRuleConfig TestConfig false

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge when happy

test_frule(config, outer, inner, rand(); frule_f=frule_via_ad, check_inferred=false)
end

@testset "Catch incorrect rules" begin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxinabox FYI there was actually a gross bug in the original implementation.

This TestConfig will be used to test rules for higher order functions f(inner, array) where we don't care that much for the rule for inner and want to do finite differencing for it out of convenience.

However, if we don't check whether a rule exists inside f/rrule_via_ad using the TestConfig, we are actually ignoring the outer rule implementation (for f) entirely, and always pass the tests because finite differencing is done on the outer rule 😅

I will wait for you to reply before merging since this was a significant development (even if not a significant change to the code)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh

Co-authored-by: Frames Catherine White <[email protected]>
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.

Add a config for testing rules that call back into AD without an frule/rrule

4 participants