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.
I don't think conditions should have kwargs in general. What's a use case for this?
There was a problem hiding this comment.
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.
I think it's fine even if not, but we should document it.
There was a problem hiding this comment.
ChainRules does not differentiate wrt kwargs, so I think we're good
There was a problem hiding this comment.
But I'd like your input on a type inference problem I found with ChainRulesTestUtils:
- on lines 107-108,
kwargsis free, it refers to an argument of the closure - on lines 110-111,
kwargsis 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.
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.
perhaps worth opening an issue there
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
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.