Question about copyArray function in mindwarp~

[ericlyon]

I just noticed that a new function was introduced, "copyArray." Looks like it's copying the array, and attempts to zero-pad. Not sure why zero pad values are set to 1.0, rather than 0.0 - looks like a bug.

In any case, I advise against using this function callout, as opposed to keeping the code within do_mindwarp(), since there may be a loss of efficiency in the function call. For improving efficiency, block operator functions memcpy() and memset() might be considered. Any thoughts?

static void copyArray(t_floatsrc, t_floatdst, size_t size, size_t zerosize) { size_t i; for(i=0; i<size; i++) dst++=src++; for(i=0; i<zerosize; i++) *dst++=1.; }

[Lucarda]

I think this question is directed to @umlaeute . (@ericlyon we have mention someone so the he/she gets email notification. Otherwise only people that read this are the "Watchers")

[umlaeute]

thanks @Lucarda :-) @erikd you probably should read a bit on markdown in order to format the code in a way that makes it fun to read :-)

about the problem at hand:

• yes that's a bug, the value should be set to 0. rather than 1.
• regarding optimisation: that would be a fine example of premature optimisation. have you done any benchmarking? (we are talking about 2 additional function calls per block, adding to a total of ~12 funcalls; this is besides a lot of loops that are doing the actual number crunching). these days, compilers are usually very smart about optimizing such things - and usually produce faster code from generic code than the feeble attempts of humans; there's a very enlightening video on the subject by matt godbolt https://www.youtube.com/watch?v=w0sz5WbS5AM). human optimisations often tend to

however, i agree that the current fix for the crash-problem is suboptimal. to recap:

• what's the problem? an out-of-bound access of arrays
• what's the ("wrong" albeit working) solution? make the arrays larger

the solution is bad insofar, as it doesn't address the actual problem, it just sweeps it under the carpet.

the real solution would be to just not access the arrays at invalid positions. there is absolutely no need to write any data into the extended portions of both the channelOne and the newChannel arrays.

we are repeatedly doing N operations on data that is never used (and should be always zero anyhow).

so my advise is to get rid of these extra operations:

• it will make the algorithm faster (than before!)
• it will make the copyArray function unneeded

thanks for bringing the issue up: i wanted to write that 2nd part anyhow, but forgot about it...

[erikd]

@umlaeute You probably should ping @ericlyon rather than me.

[ericlyon]

Thanks @umlaeute for the clarification. My concern was less about optimization (I wasn't actually planning to do anything about it), but more about complexifying the code unnecessarily for anyone who's trying to understand how it works by adding a function call, rather than doing relatively simple work inside the do_mindwarp() function. That may be a matter of programming style preference - I like to jump around as little as possible when trying to understand an algorithm. I'll fix the bug with an inline solution, unless there are any concerns about that approach.

Regarding the other point about the unnecessary calculations - I'll have to think about that. As I understand it, the algorithm stretches or shrinks the spectrum dynamically, and when it stretches, then some of the spectrum goes above the Nyquist, but without causing aliasing; in this case the above-Nyquist spectrum is just ignored. At least I think that's what's going on, such that it might actually be simplest to allow the algorithm to stand as is.

Thanks for the other advice about formatting code in github comments. That will probably come in handy for the future!

[ericlyon]

The code has now been updated. Thank you @umlaeute for discovering the bug and providing the initial fixes.

[umlaeute]

@erikd oops indeed. so many eriqs around :facepalm: sorry for the noise