Skip to content
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

Alternatives to float[] (opened as 'Implementing IEnumerable<float> on DiscreteSignal') #3

Closed
GataullinRR opened this issue Jan 17, 2019 · 6 comments
Labels

Comments

@GataullinRR
Copy link

What about implementing IEnumerable on DiscreteSignal?

@GataullinRR
Copy link
Author

Sorry. I've just noticed that this project doesn't updates for 8 months

@ar1st0crat
Copy link
Owner

ar1st0crat commented Jan 18, 2019

Hi!
Despite the sad fact that I haven't committed to this repo for a long time, I'm going to keep on working on it.

Regarding your suggestion - could you, please, give examples of scenarios when it's useful or necessary?
I guess it'll really make sense in some scenarios, and I need clear use-cases for a better understanding and code design.

This lib originally included only offline processing algorithms. I created the DiscreteSignal class just as a wrapper around plain array of floats, so that I could focus more on DSP algorithms and transforms. At this moment the library has already some online processing stuff that operates with plain arrays of floats (IOnlineFilter, BlockConvolver.Process, FirFilter.Process, IirFilter.Process). And currently signals can be created from any enumerable collections (that will ultimately be cast to array anyway. This is because indexing and BlockCopy (memcpy) are heavily used in the lib. So I think if we decide to switch to something more general, the IList interface is more appropriate than IEnumerable. And maybe it's "oldschool C++ guy" talking in me 😃, but I'm quite OK with arrays so far).

@MysteryDash
Copy link

Hello,
Adding a suggestion here instead of creating an issue since the topic is the same.
What do you think about Span? Could it be a suitable tool to enhance the performances instead of relying on IEnumerable and ToArray (which create copies)?

@ar1st0crat
Copy link
Owner

ar1st0crat commented Mar 22, 2019

Hi!

Thank you for the suggestion! Actually, I've missed some new features in C#, and now I'm quite surprised (in a good way) about Span<T> and Memory<T> types. It seems like Span could be perfectly used in some parts of the library. However, if I understand correctly, Span<T> cannot be used as the class member or a property (but it can be used in intermediate operations on stack).

In the nearest future, most likely, I'll be focused on other things, but I think Span<T> is definitely worth a try and I'll leave this issue opened.

PS. I'd also like to point out that currently memory is (re)used more or less efficiently in the library (as for me, the central part is DSP operations, not the DiscreteSignal wrapper). New memory is allocated, in most cases, only when it needs to. Two cases of unneccessary allocation/copy are:

  • wrap signal around only some part of Array;
  • wrap signal around IEnumerable, other than Array (conversion ToArray() will take place).

If these two cases become a significant bottleneck, then it's better to use plain arrays in operations (i.e. not wrap them in DiscreteSignal class).

@ar1st0crat ar1st0crat reopened this Mar 22, 2019
@ar1st0crat ar1st0crat changed the title Implementing IEnumerable<float> on DiscreteSignal Alternatives to float[] (opened as 'Implementing IEnumerable<float> on DiscreteSignal') Mar 22, 2019
@MysteryDash
Copy link

MysteryDash commented Mar 24, 2019

I'm currently using your library in a project (not really ready to be released on Github) that involves computing a lot of FFTs (between 10 to 100k) and one of the bottlenecks was creating new arrays to store the data where the FFT is computed (considering that I pre-load the entire data in a single array, it's better to point to the place where the data is rather than copying it to a new buffer).
I got a huge performance boost by simply taking your Fft.cs and replacing void Direct(float[] re, float[] im) with void Direct(Span<float> re, Span<float> im) (now, is it better to just replace them or create a brand new class for Span, I'm not really sure).
Here's the full file I'm currently using.

EDIT: As a comparison, I tried using NAudio's FastFourierTransform, but I quickly gave up seeing that it'd take minutes (with your implementation it takes around one second to process everything) because of the Complex struct used (instead of using two float arrays).

@ar1st0crat
Copy link
Owner

Yes, your scenario clearly requires Span. Out of interest, what was the performace gain? In the "no-span version" did you create only one temp buffer and copy data there each time via BlockCopy() (or NWaves.Utils.FastCopyTo()) method? I'm sure Span is faster; I'm asking, because I'm just curious.

Still, in many cases (I believe, in most cases) temporary buffers must be created anyway, since: 1) either the initial array/signal must be preserved; 2) or frames may overlap (it's common in spectral analysis). Also, as far as I understand, each particular FFT routine will run slightly faster with arrays (Span adds little overhead for inner loops in array, from what I have read). Of course, in scenarios like yours the overhead of memory copying will be much more significant, but otherwise, array version would be preferred.

So, as for adding Span support to the lib, users should be able to choose between array version and Span version, thus it's better to add a new class, just like you did. However, I'm still contemplating, should it really be done right now... I don't want to retarget project to .NET Standard 2.1 so far; neither I don't want to add dependency to System.Memory package. I guess a lot of folks use older versions of VS and .NET (just like me myself )))...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants