-
Notifications
You must be signed in to change notification settings - Fork 6
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
Prepare for 1.8 #19
Prepare for 1.8 #19
Conversation
Codecov Report
@@ Coverage Diff @@
## main #19 +/- ##
===========================================
- Coverage 100.00% 92.00% -8.00%
===========================================
Files 1 1
Lines 42 50 +8
===========================================
+ Hits 42 46 +4
- Misses 0 4 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
||
conditions_x(x̃) = conditions(x̃, y) | ||
conditions_y(ỹ) = -conditions(x, ỹ) | ||
conditions_x(x̃; kwargs...) = conditions(x̃, y; kwargs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think conditions should have kwargs in general. What's a use case for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see it used in the OT example, that's clever. You are using kwargs to indicate parameters we don't want to differentiate wrt? Is this consistent with how ChainRules handles kwargs though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine even if not, but we should document it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChainRules does not differentiate wrt kwargs, so I think we're good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'd like your input on a type inference problem I found with ChainRulesTestUtils:
- on lines 107-108,
kwargs
is free, it refers to an argument of the closure - on lines 110-111,
kwargs
is not free, it refers to the keyword arguments passed to therrule
This is a workaround I found to avoid type inference errors. The natural thing to do was to pass the keyword arguments from the rrule
directly in lines 107-108, but for some reason this gave rise to an inference error in the first round of tests (unconstrained optimization). Any clue why? Is that a problem at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced further to:
using Test, ChainRulesCore, ChainRulesTestUtils
struct FF{F, Y}
f::F
y::Y
end
(f::FF)(x) = f.f(x, f.y)
g(x, y) = x + y;
f = FF(g, rand(3))
rc = ChainRulesTestUtils.TestConfig()
x = rand(3)
@inferred rrule_via_ad(rc, f, x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps worth opening an issue there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably replace that test with @inferred
on an rrule_via_ad
call using a Zygote.ZygoteRuleConfig()
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favor of merging as-is until the upstream bug is fixed, and then release 0.3.0
@mohamed82008 should we merge this? |
Ya let's merge it. |
No description provided.