-
Notifications
You must be signed in to change notification settings - Fork 1
Type check for kwargs #22
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
- Coverage 96.52% 96.47% -0.05%
==========================================
Files 4 4
Lines 144 142 -2
==========================================
- Hits 139 137 -2
Misses 5 5 ☔ View full report in Codecov by Sentry. |
ext/WeightInitializersCUDAExt.jl
Outdated
@@ -38,6 +39,7 @@ end | |||
|
|||
function identity_init(rng::AbstractCuRNG, ::Type{T}, dims::Integer...; | |||
gain::Number=1, shift::Integer=0) where {T <: Number} | |||
gain = gain isa T ? gain : convert(T, gain) |
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 you need to check for this. The compiler is smart enough to get rid of these
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.
should we also get rid of this then?
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.
the T(gain)
specifically
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.
sorry scratch that, I misunderstood
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 meant gain isa T ? gain : convert(T, gain)
can be written as T(gain)
and the compiler should be smart to remove it if the types match.
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, should I take that approach with all of the checks?
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.
yes
There is some eyntax error, probably |
true, I forgot to fix the |
Addresses #21