-
Notifications
You must be signed in to change notification settings - Fork 193
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
Complex signalcorr #742
base: master
Are you sure you want to change the base?
Complex signalcorr #742
Conversation
45bb03f
to
9e04920
Compare
9e04920
to
f1af27e
Compare
I wonder if any of the implementations could benefit from RealDot.jl? It contains optimized implementations of |
@devmotion The function |
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)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
e73d83c
to
2efdf68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
330fefb
to
af9e1dc
Compare
a53c9db
to
d3b92e8
Compare
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 |
202ac84
to
e9e943c
Compare
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. |
e9e943c
to
25a5ea6
Compare
Rebased on the latest master, any chance of having this merged? |
25a5ea6
to
4174786
Compare
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]>
4174786
to
4f30211
Compare
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.