-
Notifications
You must be signed in to change notification settings - Fork 35
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
Clean up voltageToEfieldConverter and use exact solution if possible #758
base: develop
Are you sure you want to change the base?
Conversation
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.
nice cleanup, thanks!
I always liked to have the explicit equations in the code, but your generic analytic matrix inversion code is neater as it generalized to N dimensions. However, it might be slightly slower than the explicit implementation of the inverse for two dimensions? Then we could also catch the special case with two dimensions and use the explicit equation? Up to you.
Thanks, good point. I've added it back in (and updated the MWE above). The performance does improve by a lot. Unfortunately the bottleneck is not in this part of the voltageToEfieldConverter so we don't gain much, but I suppose it doesn't hurt either. |
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.
great, thanks
Removes some unused code from the voltageToEfieldConverter, and uses the exact solution for the matrix inversion in the case of only two antennas. This was previously discussed in #527. It is useful e.g. for LOFAR where unfolding is always done per antenna pair. The output is unchanged, but it makes things a bit quicker (the inversion is sped up by ~a factor 2)
MWE below to demonstrate that the output is unchanged: