-
Notifications
You must be signed in to change notification settings - Fork 12
Port bpf changes from PR #58. #60
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
Conversation
radae/dsp.py
Outdated
|
|
||
| # Reallocate x_mem if x_baseband size changes | ||
| if len(self.x_mem) != (len(self.mem) + len(x_baseband)): | ||
| self.x_mem = np.zeros(len(self.mem) + len(x_baseband), dtype=np.csingle) |
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 like the idea of pre-allocating x_filt, however lets do it once at init time, based on the maximum buffer length pased into the init function. Have an assert at run time to make sure we don't get passed a bigger n than we have room for.
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.
Updated to allocate only in the constructor.
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.
L75 performs a run time allocation of self.x_mem? The assert should check the length of x_mem.
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.
The idea is no run time allocation of memory, sorry if I didn't make that clear.
radae/dsp.py
Outdated
| # The advantage of operating on the strided array is that we make only one transition between Python | ||
| # and the NumPy C code, reducing overhead. | ||
| self.x_filt[0:n] = np.dot(x_mem_slided, self.h) | ||
| self.mem = self.x_mem[-self.Ntap-1:] # save filter state for next time |
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.
OK, well explained 👍
|
OK so we need a way to make sure these optimisations perform exactly the same as
|
Hopefully 08b9964 is what you meant. I emailed the results from this PR (and from |
| rx = np.cos(2*np.pi*centre_freq_Hz*np.arange(Fs_Hz)/Fs_Hz) # 1 sec real sinewave | ||
| rx_bpf = bpf.bpf(rx) | ||
| rx_bpf.tofile("complex_bpf_test1.c64") | ||
| print(rx.shape,rx_bpf.shape) |
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'm not getting sensible results when I load the emailed files into Octave:
orig_test1=load_c64('~/Downloads/complex_bpf_test/orig_complex_bpf_test1.c64',1);
orig_test1(1:10)
ans =
8.6456e-21 + 1.2816e-02i
0 - 1.2046e-04i
0 + 8.4085e-01i
0 + 8.4085e-01i
1.5336e-28 + 8.1424e-01i
2.4249e+07 + 9.5343e-01i
-3.6893e+19 - 9.3648e-01i
3.6893e+19 + 9.8109e-01i
1.9996e+21 - 1.0402e+00i
-1.0940e+18 + 8.6109e-01i
load_c64.m can be found in #42
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.
Hmm, it looks like NumPy is using complex128 and not complex64. Might explain the issues I'm having with radae_rx_basic having a slightly higher loss figure now, too. I'll experiment some more.
…tatic alloc of x_mem.
|
OK, I was able to figure out the issue with allocating (main_* = from |
|
|
||
| # Store concatenated memory and baseband samples into x_mem | ||
| np.concatenate([self.mem, x_baseband], out=self.x_mem[0:len(x_baseband) + len(self.mem)]) | ||
|
|
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.
Just an observation - I guess this is kinda where Python gets awkward and we might as well be using C as we're having to think about in place versus pointers etc 🤔 Oh well, it is what it is.
|
Thanks @tmiw. It looks like one file is empty: |
Oops, I added the write a bit too early for the second file. Try this: |
|
Looks good @tmiw 👍 Shows only some tiny differences. This sort of test could be automated in a C port, e.g. to compare C to Python, but a one off manual test is OK for this PR. |
Per previous PLT discussion, this PR ports only the BPF changes from PR #58 as this is shared with RADEV2. Please see original PR for rationale and benchmarks.