Skip to content
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

Support conv_direct! on custom datatypes #592

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

adrhill
Copy link
Contributor

@adrhill adrhill commented Jun 17, 2024

Closes #490, #405.

As described in #490, undefined array elements can't be indexed on all datatypes:

julia> v1 = Array{Float32}(undef, 3)
3-element Vector{Float32}:
 2.7761215f-21
 1.0f-45
 2.7765319f-21

julia> v2 = Array{Set}(undef, 3)
3-element Vector{Set}:
 #undef
 #undef
 #undef

julia> v1[1] # works
2.7761215f-21

julia> v2[1] # doesn't work
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getindex(A::Vector{Set}, i1::Int64)
   @ Base ./essentials.jl:13
 [2] top-level scope
   @ REPL[34]:1

Such indexing is used in the right-hand side of conv_direct! on main

y[w_idx, h_idx, d_idx, c_out, batch] = alpha*dotprod + beta*y[w_idx, h_idx, d_idx, c_out, batch]

which means that conv_direct! doesn't support all input datatypes.

However, assignments work, which is way the problem can be solved by first setting y to zero, as suggested here by @ToucheSir.

@adrhill
Copy link
Contributor Author

adrhill commented Jun 18, 2024

Looks like failures on nightly are unrelated.

@adrhill
Copy link
Contributor Author

adrhill commented Jun 19, 2024

@mcabbott could a patch release be tagged for this?

@pxl-th pxl-th merged commit d423576 into FluxML:master Jun 19, 2024
11 of 13 checks passed
@adrhill adrhill deleted the ah/conv-direct-custom branch June 19, 2024 19:46
@pxl-th
Copy link
Member

pxl-th commented Jun 19, 2024

Tagged 0.9.18 release: 62f6074

@adrhill
Copy link
Contributor Author

adrhill commented Jun 19, 2024

Thanks!

@@ -81,6 +81,11 @@ function conv_direct!(
# Use `calc_padding_regions` to determine where we do or don't need to worry about padding
padded_regions, central_region = calc_padding_regions(cdims)

# Set outputs to zero to support custom datatypes (https://github.com/FluxML/NNlib.jl/issues/490)
if iszero(beta)
y = fill!(y, zero(yT))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the original issue, naive fill!/fill doesn't necessarily work with e.g. BigInt:

julia> a = Vector{BigInt}(undef, 2)
2-element Vector{BigInt}:
 #undef
 #undef

julia> fill!(a, zero(BigInt))
2-element Vector{BigInt}:
 0
 0

julia> a[1] ===a[2]
true

I don't know how important these kinds of usecases are for NNlib.jl though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that problematic here?

julia> a = Vector{BigInt}(undef, 2)
2-element Vector{BigInt}:
 #undef
 #undef

julia> fill!(a, zero(BigInt))
2-element Vector{BigInt}:
 0
 0

julia> for (i, ai) in enumerate(a)
         a[i] = i + 0 * a[i]
       end

julia> a
2-element Vector{BigInt}:
 1
 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when using Float64: ERROR: UndefRefError: access to undefined reference
5 participants