mlverse / torchaudio

R interface to torchaudio
https://mlverse.github.io/torchaudio/
Other
26 stars 6 forks source link

Test failures #51

Closed skeydan closed 1 year ago

skeydan commented 1 year ago

This fixes all tests but one: https://github.com/curso-r/torchaudio/actions/runs/3827989720/jobs/6513085424#step:8:159

As to the remaining one, I suspect this could be an issue with indexing in torch. Comparing, 1:1, the R and Python versions of flanger, I see the first difference appearing here:

https://github.com/pytorch/audio/blob/d0dca11546567bc96886e9965224e4e6681170ba/torchaudio/functional/filtering.py#L838

https://github.com/curso-r/torchaudio/blob/3667cc044e0fcd8ae7b68a7a9343ee1537ea686d/R/functional.R#L1540

Extracting the relevant code, we have this:

Python:

delay_bufs = torch.arange(1,10).view(1,3,3)
delay_bufs
# tensor([[[1, 2, 3],
#          [4, 5, 6],
#          [7, 8, 9]]])

channel_idxs = torch.tensor([0,1,2])
delay_buf_pos = 2
int_delay = torch.tensor([2,2,3])
delay_buf_length = 3
third = (delay_buf_pos + int_delay) % delay_buf_length
third
# tensor([1, 1, 2])

delay_bufs[:, channel_idxs, third]
# tensor([[2, 5, 9]])

R:

delay_bufs <- torch_arange(1,9)$view(c(1,3,3))
delay_bufs
# torch_tensor
# (1,.,.) = 
# 1  2  3
# 4  5  6
# 7  8  9
# [ CPUFloatType{1,3,3} ]
channel_idxs <- torch_tensor(c(0,1,2), dtype = torch_long())
delay_buf_pos <- 2L
int_delay <- torch_tensor(c(2,2,3), dtype = torch_long())
delay_buf_length <- 3L
third <- (delay_buf_pos + int_delay) %% delay_buf_length
third
#torch_tensor
# 1
# 1
# 2
# [ CPULongType{3} ]
delay_bufs[ , channel_idxs + 1L, third + 1L]
# torch_tensor
# (1,.,.) = 
# 2  2  3
# 5  5  6
# 8  8  9
# [ CPUFloatType{1,3,3} ]

@dfalbel what do you think? With [advanced] indexing, due to my "spatial handicap", I really have awful problems ...

dfalbel commented 1 year ago

This indeed looks like a bug in torch. Looks like we don't correctly handle the case where two tensors are passed in the indexes. It seems that what happens in PyTorch is that we don't correctly treat the tensors as coordinates that we want to extract.

Actually, I just found a note here

Note: Starting from version 0.5.0, vector indexing in torch follows R semantics, prior to that the behavior was similar to numpy’s advanced indexing. To use the old behavior, consider using ?torch_index, ?torch_index_put or torch_indexput.

Maybe this works?

torch_index(delay_bufs[1,], list(channel_idxs + 1L, third+1L))
skeydan commented 1 year ago

Actually, I just found a note here

it's "read the docs", right? :-)) Thanks so much, that works (with adaptations for the situation)!

skeydan commented 1 year ago

All tests passing now :-)

skeydan commented 1 year ago

I would normally try to not do it, but I'll be adding commits due to

https://github.com/curso-r/torchaudio/issues/46

here as well, since some of them depend on this branch.

skeydan commented 1 year ago

OK, done :-) I guess this would be ready for a release, then? (See also https://github.com/curso-r/torchaudio/issues/46)