StatsBase.jl icon indicating copy to clipboard operation
StatsBase.jl copied to clipboard

Complex signalcorr

Open kagalenko-m-b opened this issue 4 years ago • 7 comments

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.

kagalenko-m-b avatar Nov 30 '21 09:11 kagalenko-m-b

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

devmotion avatar Nov 30 '21 12:11 devmotion

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

kagalenko-m-b avatar Nov 30 '21 15:11 kagalenko-m-b

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: https://github.com/JuliaStats/StatsBase.jl/pull/742#discussion_r759381162).

devmotion avatar Nov 30 '21 16:11 devmotion

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

nalimilan avatar Dec 28 '21 15:12 nalimilan

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 avatar Jan 13 '22 15:01 kagalenko-m-b

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 avatar Jul 05 '22 15:07 kagalenko-m-b

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

kagalenko-m-b avatar Oct 27 '22 16:10 kagalenko-m-b