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

Add ReverseSequence #698

Merged
merged 10 commits into from
Jun 16, 2024
Merged

Add ReverseSequence #698

merged 10 commits into from
Jun 16, 2024

Conversation

NeroBlackstone
Copy link
Contributor

I don't know if this is the correct implementation

Copy link
Member

@avik-pal avik-pal left a comment

Choose a reason for hiding this comment

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

Let's redesign the interface a bit. I would default the dims to nothing. The behavior for that would be:

  1. If the input is AbstractVector{T} and !isbitstype(T) then we simply call Iterators.reverse (note this is lazy)
  2. For bitstype AbstractVector, call reverse(x).
  3. For other arrays, it calls reverse(x; dims=ndims(x) - 1).

This essentially prevents unnecessary copies. For eg, for Bidirectional RNN we can simply construct an iterator once and make a lazy reverse iterator using this API without copying the entire array.

If dims is specified,

  1. AbstractVector{T} and !isbitstype(T) throws an error
  2. All other array inputs call reverse(x; dims=r.dims)

src/layers/basic.jl Outdated Show resolved Hide resolved
@NeroBlackstone
Copy link
Contributor Author

If dims is specified,

  1. AbstractVector{T} and !isbitstype(T) throws an error

Hi, I'm curious why isbitstype() is needed here. For any 1D array, we can't specify dim right? So why we need isbitstype(), it would be nice if you could give an example with specific code. Thank you very much.

@NeroBlackstone
Copy link
Contributor Author

I have updated the implementation according to your guidance, except for the doubtful points above.

@NeroBlackstone
Copy link
Contributor Author

test failed on gpu, I can't reproduce on local since I don't have AMDGPU or CUDA devices...

@ludvigk
Copy link

ludvigk commented Jun 13, 2024

I think it fails because Base.Iterators.Reverse{CuArray{Float32, 1, CUDA.Mem.DeviceBuffer}} is not isbitstype. CUDA doesn't like that. Even a minimal example like this fails for that reason:

g = CUDA.zeros(1)
gr = Iterators.reverse(g)
g .* gr

@ludvigk
Copy link

ludvigk commented Jun 13, 2024

Using reverse instead of Iterators.reverse on arrays of type GPUArraysCore.AbstractGPUArray should fix this.

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
@NeroBlackstone
Copy link
Contributor Author

Using reverse instead of Iterators.reverse on arrays of type GPUArraysCore.AbstractGPUArray should fix this.

I got a PC with GPU and test locally.

I found Float64 works well but Int throws errors. Weird...

@avik-pal
Copy link
Member

Add this layer to the docs, also rebase with main. That will fix the doc build

src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
src/layers/basic.jl Outdated Show resolved Hide resolved
@avik-pal avik-pal force-pushed the main branch 5 times, most recently from 25325ea to e931a5e Compare June 16, 2024 02:10
Copy link

codecov bot commented Jun 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.13%. Comparing base (acd924f) to head (dbeb848).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
- Coverage   87.34%   81.13%   -6.21%     
==========================================
  Files          50       50              
  Lines        2505     2513       +8     
==========================================
- Hits         2188     2039     -149     
- Misses        317      474     +157     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NeroBlackstone
Copy link
Contributor Author

I noticed some test failures, should I follow up on this PR? Or leave all the rest to you?

@avik-pal
Copy link
Member

Github won't let me edit files directly. So if you add the docs to https://github.com/LuxDL/Lux.jl/blob/main/docs/src/api/Lux/layers.md#misc-helper-layers the build will pass

@avik-pal avik-pal merged commit 05c4d02 into LuxDL:main Jun 16, 2024
68 of 76 checks passed
@NeroBlackstone NeroBlackstone deleted the Reverse branch June 16, 2024 08:58
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.

3 participants