-
Notifications
You must be signed in to change notification settings - Fork 20
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
testing ic against py_ic #266
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.
A few comments but otherwise looks good.
modules/lib_ic/api/ic_state.h
Outdated
@@ -177,7 +177,7 @@ typedef struct { | |||
bfp_s32_t error_bfp[IC_Y_CHANNELS]; | |||
/** Storage for Error and error mantissas. Note IFFT is done in-place | |||
* so the Error storage is reused for error. */ | |||
complex_s32_t DWORD_ALIGNED Error[IC_Y_CHANNELS][IC_FD_FRAME_LENGTH]; |
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.
Is there a reason for changing this? Since this is 'Error' and not 'error', complex_s32_t seems more intuitive, although IDFT is done in place and same memory stores error so not a big deal I guess
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 did it for the tests and forgot to change it back. I agree that the complex_s32_t makes more sense with this name
@@ -257,7 +293,8 @@ void ic_adaption_controller(ic_state_t *state, uint8_t vad){ | |||
|
|||
const float_s32_t one = {1, 0}; | |||
const float_s32_t zero = {0, 0}; | |||
const float_s32_t delta = {1100, -40}; //1100 * 2**-40 = 0.000000001 (from stage_b.py) | |||
//const float_s32_t delta = {1100, -40}; //1100 * 2**-40 = 0.000000001 (from stage_b.py) | |||
const float_s32_t delta = {352, -45}; //352 * 2**-45 ~ 0.00000000001 |
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.
Is there a reason for changing this? Could you add a comment here?
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.
This delta is being used to prevent zero division. In the test case I saw, the denominator was 1.7e-8 while the previous delta was 1e-9. In some cases, it was significant enough to make the fast ratio more than 1 where it shouldn't be so. The test was failing with this delta. I've changed it in both C and python to the less significant one 1e-11, and the test has passed. I'll add a comment to cover that.
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.
That makes sense. Since we're changing an IC parameter, do ensure that the full pipeline tests keyword results don't show anything unexpected.
@@ -213,14 +215,22 @@ def get_suppression_arr(input_audio, output_audio): | |||
for i in np.arange(0, num_frames*frame_advance, frame_advance): | |||
i = int(i) | |||
in_rms = rms(input_audio[0][i:i+frame_advance]) | |||
out_rms = rms(output_audio[0][i:i+frame_advance]) |
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.
You could do something like, out_rms = out_rms.reshape(1 , out_rms.shape[-1]) to force it into a 2D shape. Then you'll not need the if condition below.
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 output_audio can either be a single channel or have two channels. This will work fine for a single-channel array. However, I can't reshape a (2, 240) array into a (1, 240) array, python will give me an error. So I'll have to have an if ether way
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.
Right, that makes sense. I thought the output was either shaped (n,) or (1,n).
@@ -273,7 +283,11 @@ def check_delay(record_property, test_case, input_audio, output_audio): | |||
output_audio = np.asarray(output_audio, dtype=float) / np.iinfo(np.int32).max | |||
|
|||
frame_in = input_audio[0][:frame_advance*2] | |||
frame_out = output_audio[0][:frame_advance*2] |
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.
Same as above
No description provided.