Complex signalcorr #742

kagalenko-m-b · 2021-11-30T09:36:04Z

This PR aims to fix the issue #459. It mostly replaces functions' input type signatures by the less restrictive ones. To produce test vectors for autocovariance and autocorrelation, I added the file rcall_test.jl that defines test functions, relying on R via RCall.jl.

I also fixed durbin!() function so that it is now valid for complex inputs, and avoids the need for repeated invocation by keeping the results for the succession of solved linear systems.

src/signalcorr.jl

test/signalcorr.jl

src/toeplitzsolvers.jl

src/signalcorr.jl

devmotion · 2021-11-30T12:12:58Z

I wonder if any of the implementations could benefit from RealDot.jl? It contains optimized implementations of real(dot(x, y)).

kagalenko-m-b · 2021-11-30T15:57:05Z

@devmotion The function demean_col() should be considered broken and replaced because of this .

devmotion · 2021-11-30T16:08:55Z

Not sure why you pinged me but this seems unrelated to this PR and not specific to complex inputs? It would be good to keep the PR minimal and just make the changes that are needed to support complex numbers. I would prefer if other issues are discussed separately or directly fixed in separate PRs. It would be important as well to keep all existing tests to ensure that no existing functionality is broken (as I mentioned also in this comment: #742 (comment)).

src/signalcorr.jl

devmotion

I had another look and my impression is that apart from the changes of the input types most changes should not be needed. The existing code for the matrix case is already efficient and reuses a cache, so allocations should not be reduced by this PR (in contrast, for the vector case it will always allocate an array now but doesn't currently).

src/signalcorr.jl

nalimilan

Thanks!

Could you add tests for when the input array contains missing? Previously this was an error, but now that the signature is less strict we should ensure we don't return misleading results.

src/signalcorr.jl

test/signalcorr_complex.jl

test/rcall_test.jl

src/toeplitzsolvers.jl

test/signalcorr_complex.jl

nalimilan · 2021-12-28T15:16:48Z

Thanks, but you don't seem to have answered all my questions/comments.

kagalenko-m-b · 2022-01-13T15:23:39Z

Thanks, but you don't seem to have answered all my questions/comments.

I have not resolved this and this conversations, because I am not sure how to proceed. Those are similar - I added functions to generate complex test vectors. On one hand, those functions are not necessary to run the tests, they can be replaced by their outputs, and the one with RCall isn't used by the tests at all. On the other hand, they document the origin of the test vectors. For the second of those, it is present only in the complex case, because the real case already has test vectors. So, as a maintainer, please indicate your preference for the best way to resolve this. It can be as simple as deleting those parts from PR, because they are really a kind of documentation and need not to be run.

kagalenko-m-b · 2022-07-05T15:57:29Z

Thanks, but you don't seem to have answered all my questions/comments.

In the latest revision I have removed all parts that caused questions. Now this PR is streamlined - it does nothing other than correct function signatures and revise the Durbin algorithm to make it valid for complex inputs. The tests with the nightly Julia are failing, but not on the parts that I touched.

kagalenko-m-b · 2022-10-27T16:54:50Z

Rebased on the latest master, any chance of having this merged?

Co-authored-by: Moritz Schauer <[email protected]>

Implement assorted suggestions from code review Co-authored-by: David Widmann <[email protected]> Co-authored-by: Moritz Schauer <[email protected]>

Co-authored-by: David Widmann <[email protected]>

kagalenko-m-b force-pushed the complex_signalcorr branch from 45bb03f to 9e04920 Compare November 30, 2021 09:46