-
-
Notifications
You must be signed in to change notification settings - Fork 609
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
Debugging messages about type mismatches #1031
Conversation
I'm a big fan of providing users with the option to get a better hold of performance and debugging that comes through so easily, thanks @mcabbott !! Could you please do a quick rebase? |
Sure. I guess a remaining question is whether these want to be a bigger warning, say |
From your comment on the other issue, you'd shown a significant speedup that could have been avoided if we could be type stable, which makes it critical. Having tooling around being able to find these gotchas I think is a more stable and longer term useful situation, so as long as people can access it easily, without performance penalties to the actual runtime, I think is a fair deal, so a warn, though very tempting, might be unnecessary. |
Could do with tests that demonstrate the cases this catches and ones it misses |
Perhaps this is ready? What it doesn't do is tell you which layer of a Chain messes up the forward pass. Nor will it catch at all if a particular layer messes up the backward pass, although that doesn't seem to be common. Edit: what it also doesn't do is flag similar conversions of input type in Lines 73 to 74 in d4cf143
Although you will still get the "Chain(...) receives input of eltype Float64 but creates output of eltype Float32" warning. |
Pinging @MikeInnes for his thoughts on this |
@MikeInnes pinging again as this would really help, e.g. with hunting down obscure bugs like #965 |
It'll have to be a fresh PR as apparently rebasing has disabled the "reopen" button. IMO this would still be worth doing, and should be a warning not an |
@CarloLucibello seemed active last days. |
https://discourse.julialang.org/t/flux-con-warning/49456 is today’s example of a nice friendly this-will-be-slow warning. |
This adds a few warning messages about conversions to Float64 or worse, which seem to be a common problem, today #1026, last week #815.
These are behind
@debug
, which I see is also used by NNlib for some such warnings. Another option would be to make these errors, controlled by someFlux.allowconvert(false)
along the lines of what CuArrays does for scalar access, see earlier commit.I'm not sure they will catch all the activation-function bugs mentioned in
performance.md
. Perhaps it would be better to attach the warning to every layer, not toChain
.