-
Notifications
You must be signed in to change notification settings - Fork 63
Make add!! decide if mutable or not #234
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
Conversation
|
||
function debug_add!(accumuland, t::InplaceableThunk) | ||
returned_value = t.add!(accumuland) | ||
if returned_value !== accumuland |
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 this is cheap enough to do it outside of debug_mode
?
Maybe if we find outselves having to tell people to turn on debug_mode
to find errors we should look into that?
Or maybe we should look into it now.
What do you think?
Does this invalidate It does invalidate this one |
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.
A couple suggestions for improvements on the user-facing side, otherwise looks really good
|
||
## Features of Debug Mode: | ||
|
||
- If you add a `Composite` to a primal value, and it was unable to construct a new primal values, then a better error message will be displayed detailing what overloads need to be written to fix 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.
detailing what overloads need to be written to fix this
Is this true? this suggest to me it'll print something like
"""
If `Foo` can be updated in place, define `ChainRulesCore.is_inplaceable_destination(::Foo) = true
"""
But as far as i can see that's not in the error message, or did i miss it (or misread this comment)?
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.
This is actually documenting something older.
Its unrelated to the main content of this PR.
Not inplace add but simply +
Its this message
ChainRulesCore.jl/src/differentials/composite.jl
Lines 269 to 284 in a3e76b1
function Base.showerror(io::IO, err::PrimalAdditionFailedException{P}) where {P} | |
println(io, "Could not construct $P after addition.") | |
println(io, "This probably means no default constructor is defined.") | |
println(io, "Either define a default constructor") | |
printstyled(io, "$P(", join(propertynames(err.differential), ", "), ")", color=:blue) | |
println(io, "\nor overload") | |
printstyled(io, | |
"ChainRulesCore.construct(::Type{$P}, ::$(typeof(err.differential)))"; | |
color=:blue | |
) | |
println(io, "\nor overload") | |
printstyled(io, "Base.:+(::$P, ::$(typeof(err.differential)))"; color=:blue) | |
println(io, "\nOriginal Exception:") | |
printstyled(io, err.original; color=:yellow) | |
println(io) | |
end |
I just realized when writing this this PR that I there was no list of what Debug Mode did,
so I couldn't add the Inplace behavour to it.
Those integration test failures look like they need to be looked into |
Closes #232