org-arl / SignalAnalysis.jl

Signal analysis toolbox for Julia
MIT License
42 stars 11 forks source link

Define `vec(s::SampledSignal)` #33

Closed ymtoo closed 1 year ago

ymtoo commented 1 year ago

reshape(s::SampledSignal, args...) is undefined and hence the sampling rate is not retained:

julia> s = signal(randn(1000,1), 100)
SampledSignal @ 100.0 Hz, 1000×1 Matrix{Float64}:
 -1.1582512154996183
 -1.6624304955859712
  1.013253308849691
  ⋮
  0.555604245357521
 -0.3619263760688517

julia> reshape_s = reshape(s, :)
1000-element reshape(MetaArray(::Matrix{Float64}, (:fs,)), 1000) with eltype Float64:
 -1.1582512154996183
 -1.6624304955859712
  1.013253308849691
  ⋮
  0.555604245357521
 -0.3619263760688517

julia> vec_s = vec(s)
1000-element reshape(MetaArray(::Matrix{Float64}, (:fs,)), 1000) with eltype Float64:
 -1.1582512154996183
 -1.6624304955859712
  1.013253308849691
  ⋮
  0.555604245357521
 -0.3619263760688517

julia> framerate(s)
100.0f0

julia> framerate(reshape_s)
1.0

julia> framerate(vec_s)
1.0

After the change:

julia> framerate(reshape_s)
100.0f0

julia> framerate(vec_s)
100.0f0
codecov-commenter commented 1 year ago

Codecov Report

Base: 73.31% // Head: 73.44% // Increases project coverage by +0.12% :tada:

Coverage data is based on head (2535c9e) compared to base (d5c7d77). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #33 +/- ## ========================================== + Coverage 73.31% 73.44% +0.12% ========================================== Files 10 10 Lines 622 625 +3 ========================================== + Hits 456 459 +3 Misses 166 166 ``` | [Impacted Files](https://codecov.io/gh/org-arl/SignalAnalysis.jl/pull/33?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=org-arl) | Coverage Δ | | |---|---|---| | [src/signals.jl](https://codecov.io/gh/org-arl/SignalAnalysis.jl/pull/33/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=org-arl#diff-c3JjL3NpZ25hbHMuamw=) | `42.66% <100.00%> (+2.38%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=org-arl). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=org-arl)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mchitre commented 1 year ago

What would you want reshape() to mean for a signal? The signal is meant to be samples x channels, so how does one meaningfully reshape it?

ymtoo commented 1 year ago

Agreed. A general reshape(::SampledSignal, dims...) doesn't have any meaning, i.e., rows are samples and columns are channels. My only concern is that vec 1D and 2D 1-channel signals return outputs with different types and different sampling rates:

julia> s1 = signal(randn(1000), 100); # 1D

julia> vs1 = vec(s1) # fall back to vec(a::AbstractVector) = a
SampledSignal @ 1.0 Hz, 1000-element Vector{Float64}:
 -1.1535828743870096
  0.7432547241034211
  0.5487126501374742
  ⋮
 -1.4863960533514835
 -0.0547328132284642
 -0.6737388579945773

julia> framerate(vs1) # sampling rate is ratained
100.0f0

julia> s2 = signal(randn(1000,1), 100); # 2D

julia> vs2 = vec(s2) # falls back to vec(a::AbstractArray) = reshape(a,length(a))
1000-element reshape(MetaArray(::Matrix{Float64}, (:fs,)), 1000) with eltype Float64:
 -0.9553796055933544
  1.2677297653623156
 -1.1623895153949142
  ⋮
 -1.2483153724672156
  1.854504355773361
 -0.23962676217559373

julia> framerate(vs2) # sampling rate is not retained
1.0
mchitre commented 1 year ago

Maybe define vec() for sampled signals, and only succeed if the signal was single channel in the first place?

ymtoo commented 1 year ago

Sounds good. reshape(s::SampledSignal, args...) is replaced by vec(s::SampleSignal). If s is either a 1D sampled signal or 2D single-channel sampled signal, a 1D sampled signal is returned. Else, a vector of numbers is returned.

julia> xlen = 8000;

julia> x = randn(xlen);

julia> fs = 1000;

julia> s0 = signal(x, fs)
SampledSignal @ 1000.0 Hz, 8000-element Vector{Float64}:
 -0.8235858456691025
 -0.47525648185689523
 -2.638623590833338
  ⋮
  0.7988062854651686
 -0.792633013796295

julia> vec(s0) == s0
true

julia> framerate(vec(s0)) == fs
true

julia> size(vec(s0)) == (xlen,)
true

julia> s1 = signal(reshape(x, :, 1), fs)
SampledSignal @ 1000.0 Hz, 8000×1 Matrix{Float64}:
 -0.8235858456691025
 -0.47525648185689523
 -2.638623590833338
  ⋮
  0.7988062854651686
 -0.792633013796295

julia> vec(s1) == s0
true

julia> framerate(vec(s1)) == fs
true

julia> size(vec(s1)) == (xlen,)
true

julia> x2 = randn(8000, 2);

julia> s2 = signal(x2, fs)
SampledSignal @ 1000.0 Hz, 8000×2 Matrix{Float64}:
 -0.158859    0.829052
  1.08812     0.0729886
  0.287631   -0.514156
  ⋮          
 -0.399386   -0.705283
 -0.0755832  -1.51299

julia> vec(s2) == vec(x2)
true

julia> x3 = randn(8000, 1, 1);

julia> s3 = signal(x3, fs)
SampledSignal @ 1000.0 Hz, 8000×1×1 Array{Float64, 3}:
[:, :, 1] =
 -1.0129234810207155
 -3.1751794202789134
 -1.8097055483900717
  ⋮
 -0.5641216706358804
  0.571156388959271

julia> vec(s3) == vec(x3)
true
mchitre commented 1 year ago

Might be better to throw an error for multi-channel signals rather than return a different data type. A user should explicitly do vec(samples(x)) if he wants an array back.

ymtoo commented 1 year ago

Throwing an error for multi-channel signals:

function Base.vec(s::SampledSignal) 
  if ndims(s) < 3 && isone(size(s, 2))
    signal(reshape(samples(s), length(s)), framerate(s))
  else
    throw(ArgumentError("reshape a multi-channel signal as a single-channel signal is undefined."))
  end 
end

triggers an error for energy(::AbstractMatrix):

julia> s = signal(randn(8000,2), 1000);

 julia> energy(s)
ERROR: ArgumentError: reshape a multi-channel signal as a single-channel signal is undefined.
Stacktrace:
 [1] vec
   @ ~/Projects/SignalAnalysis.jl/src/signals.jl:292 [inlined]
 [2] energy(s::MetaArrays.MetaArray{Matrix{Float64}, SignalAnalysis.SamplingInfo, Float64, 2}; fs::Float32)
   @ SignalAnalysis ~/Projects/SignalAnalysis.jl/src/basic.jl:8
 [3] energy(s::MetaArrays.MetaArray{Matrix{Float64}, SignalAnalysis.SamplingInfo, Float64, 2})
   @ SignalAnalysis ~/Projects/SignalAnalysis.jl/src/basic.jl:8
 [4] top-level scope
   @ REPL[48]:1

julia> sumabs2_s = sum(abs2, s; dims=1)
SampledSignal @ 100.0 Hz, 1×2 Matrix{Float64}:
 7948.15  8096.66

Since sumabs2_s = sum(abs2, s; dims=1) returns a multi-channel sampled signal, vec(sumabs2_s) throws an error. A possible fix:

energy(s::AbstractMatrix; fs=framerate(s)) = vec(samples(sum(abs2, s; dims=1)))./inHz(fs)
mchitre commented 1 year ago
  1. Perhaps signal(vec(samples(s)), framerate(s)) instead of signal(reshape(samples(s), length(s)), framerate(s))?
  2. The energy() fix you suggest seems good.
mchitre commented 1 year ago

@ymtoo do assign me as a reviewer when you're ready for review/merge

ymtoo commented 1 year ago

@mchitre I'm not able to assign a reviewer as Reviewers in the right sidebar is disabled. The PR is ready to be reviewed/merged.