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

Fix vectorization for stft #18

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Fix vectorization for stft #18

merged 1 commit into from
Aug 30, 2023

Conversation

jonatanklosko
Copy link
Member

Currently vectorization results in an error:

** (CompileError) /Users/jonatanklosko/git/nx_signal/lib/nx_signal.ex:359: the do-block in while must return tensors with the same shape, type, and names as the initial arguments.

{
 <<<<< Body (do-block) <<<<<
 #Nx.Tensor<
   vectorized[batch: 1]
   f32[3001][400]
 >
 ==========
 #Nx.Tensor<
   f32[3001][400]
 >
 >>>>>     Initial     >>>>>
 , #Nx.Tensor<
   s64
 >, #Nx.Tensor<
   s64
 >, #Nx.Tensor<
   vectorized[batch: 1]
   f32[480400]
 >}

Is this something we should relax in Nx? cc @josevalim

@josevalim
Copy link
Contributor

I'd prefer to keep them the same as vectorized and not will lead to different semantics. @polvalente decides though.

@josevalim
Copy link
Contributor

Btw, is this something that should have been caught by a test?

@polvalente
Copy link
Collaborator

I think that Nx is correct and this fix is needed here in NxSignal as-is :)

@polvalente
Copy link
Collaborator

Btw, is this something that should have been caught by a test?

Maybe, but we don't generally test for vectorization on stuff outside of Nx.

For instance, there's no explicit support for vectorization in Scholar nor Axon

@jonatanklosko jonatanklosko merged commit e387d0d into main Aug 30, 2023
2 checks passed
@jonatanklosko jonatanklosko deleted the jk-vectorize branch August 30, 2023 21:22
@josevalim
Copy link
Contributor

i mean, not really testing the vectorization, but to test stft works with vectorized tensors which would help regressions on this code in the future.

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