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 RuleConfig #45

Merged
merged 5 commits into from
Sep 11, 2021
Merged

Add RuleConfig #45

merged 5 commits into from
Sep 11, 2021

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Sep 9, 2021

This wants to add methods for rrule_via_ad and frule_via_ad from ChainRules.

Need to figure out how to test this; so far rrule_via_ad does seem to work. Not sure anything is in the right place.

@simeonschaub
Copy link
Member

Testing should now be set up, so you'll probably want to rebase this.

@simeonschaub
Copy link
Member

Would it be possible to add a test specifically making use of this?

@mcabbott
Copy link
Member Author

Yes was just trying to figure out how (and where). I've added something, but my frule_via_ad definition is clearly completely wrong.

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2021

Codecov Report

Merging #45 (48460e7) into main (0a98045) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   55.80%   55.98%   +0.17%     
==========================================
  Files          21       21              
  Lines        2100     2106       +6     
==========================================
+ Hits         1172     1179       +7     
+ Misses        928      927       -1     
Impacted Files Coverage Δ
src/runtime.jl 58.33% <100.00%> (+3.78%) ⬆️
src/stage1/forward.jl 88.40% <100.00%> (+0.34%) ⬆️
src/stage1/generated.jl 76.30% <100.00%> (+0.22%) ⬆️
src/extra_rules.jl 42.10% <0.00%> (+0.43%) ⬆️

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 0a98045...48460e7. 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.

You can test this using ChainRulesTestUtils if you wanted.
It supports testing these cases.

This basically LGTM.
I see no reason not to merge it once tests pass

test/runtests.jl Outdated Show resolved Hide resolved
src/stage1/forward.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member

CI failure is JuliaLang/julia#42125, will push a fix shortly if no one beats me to it.

@mcabbott mcabbott marked this pull request as ready for review September 10, 2021 14:57
@mcabbott mcabbott merged commit e2cfd85 into JuliaDiff:main Sep 11, 2021
@mcabbott mcabbott deleted the ruleconfig branch September 11, 2021 00:14
@simeonschaub
Copy link
Member

Just as a small comment: I'd generally always squash-merge PRs like this unless the commits are actually standalone changes.

@mcabbott
Copy link
Member Author

Sorry, will do, next time.

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.

4 participants