Skip to content

Add signal class and helper function #24

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

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Add signal class and helper function #24

merged 1 commit into from
Feb 10, 2017

Conversation

mgeier
Copy link
Member

@mgeier mgeier commented Dec 11, 2016

For now, this is just a proof of concept.

Alternatives are very welcome.
And suggestions for improvements, too.

For now, there are no doc changes yet. I'll do that if we decide to go that way ...

@trettberg
Copy link
Collaborator

What is supposed to happen when a function receives multiple signals with different sampling rates?

  • raise an error?
  • perform automatic resampling?

@mgeier
Copy link
Member Author

mgeier commented Dec 11, 2016

@trettberg Both behaviors are possible choices, we can decide once we actually have a function that takes more than one signal. We could even add an argument that specifies if resampling should be used or not.

@trettberg
Copy link
Collaborator

trettberg commented Dec 11, 2016

My line of thought was something like:
If different sampling rates are not handled automatically, we don't have to store them with the signals.
The constant in defs might be enough?

EDIT:
Okay, sample rate per signal is necessary. Otherwise the delays are meaningless.

@mgeier
Copy link
Member Author

mgeier commented Dec 16, 2016

@trettberg I agree with your edited comment. The sampling rate could be implicit (only handled by defs), but I think this has a high potential for errors.
I think it's best to make it explicit and obligatory. This is the case in this very PR.

However, a signal class could be created without explicit sampling rate and an explicit sampling rate could be used without a signal class.

As I said, IMHO the sampling rate should always be explicit, but there could still be a better alternative for the signal class proposed here.

Are there any counter-proposals?

@trettberg
Copy link
Collaborator

From what i've tried so far, this works nicely for keeping track of processing delays.
I'm with it!

A suggestion for later:
IMHO this begs for overloading the class's arithmetic operators.
For example, adding driving signals for two virtual sources with d1 + d2 should take care of time offsets (and sample rates).

@mgeier
Copy link
Member Author

mgeier commented Dec 18, 2016

I'm not sure if overloading the operators is the right way to go ... but I also don't know if it will be even needed, we'll see about that in the future.

I don't think it's a good idea to build too many features into the Signal class which would in the end lead to something like http://www.briansimulator.org/docs/reference-hears.html#sounds.

We should definitely consider using free functions instead of methods and overloaded operators.
If we use methods, we create a lock-in for our users, since they are forced to convert to a Signal object before they can do anything useful with it.
Overloaded operators might have to deal with some strange edge cases like:

somedata = ...
sig1 = somedata, 44100
sig1 = sfs.util.as_signal(sig1)
sig2 = sig1 + ([1], 44100, 0.007)

I don't think this is the way we want to go, but it is definitely a possibility ....
Consider, OTOH, using a (hypothetical) free function:

somedata = ...
sig1 = somedata, 44100
sig2 = sfs.util.add_signals(sig1, ([1], 44100, 0.007))

I think this is better, but it would probably be even better not having to add signals at all.

Since the main feature of the proposed signal class is the delay, we should probably rename it to something more specific?
What about DelayedSignal and as_delayed_signal()?
This should show that it is not a generic signal class (with many features) but rather a very specialized thing.

@mgeier
Copy link
Member Author

mgeier commented Dec 20, 2016

I switched to DelayedSignal and updated the documentation.

Any comments? Suggestions for improvements?

@trettberg
Copy link
Collaborator

Superposition of signals is necessary e.g. to generate driving functions for data-based SFS.
Acoustic room modeling (think image sources) is another application.

I do not suggest to stuff arbitrary functionality in the class but to get the math right:
For ease of illustration, assume all signals have

  • the same number of channels C,
  • a sampling rate of 1,
  • integer delays.

These signals live in the same (C x N)-dimensional vector space, for some N large enough.
The delay offset is merely an implementation detail to avoid lots of zeros.

Compare to the sparse matrix classes which (of course) do matrix arithmetics.

@mgeier
Copy link
Member Author

mgeier commented Dec 22, 2016

OK, we might need signal superposition at some point, but is it something that has influence on the current PR?

I think we can decide later if we use operator overloading or not.

But the decision if we use integer or fractional delays indeed has a lot of influence here.
As far as I understood, we want fractional delays, right?

The relation to sparse matrices is very interesting, but as you say, it's only valid for integer delays.

And even then, there is the difference that we need a way to align the signal data on the time axis.

Another detail is that our signals (currently) cannot have "holes". If you add a signal that is non-zero in the first second to another signal that only plays in the 10th second, you'll have to fill the time in-between with zeros.

The delay offset is merely an implementation detail to avoid lots of zeros.

No, it's not.
It would be if we only used positive delays, but it is actually required for negative delays. Or we would need to set some arbitrary maximum negative delay somewhere.
Or do you know a different way how we could implement this?

Another possibility would be to use one delay value per channel instead of one global delay for all channels.
Instead of (data1, fs, -0.007), we could have something like (data2, fs, [0.02, -0.005, 0.006, 0.001, -0.007]) (assuming 5 channels).
We could even make it optional: A scalar value would be applied to all channels, a sequence would have to have the right length and its elements would be applied column-wise.

What about that?

But then we also might think about having numeric weights per channel instead of applying the weights destructively ...

@mgeier mgeier changed the title Add Signal class and as_signal() Add signal class and helper function Dec 23, 2016
@trettberg
Copy link
Collaborator

OK, we might need signal superposition at some point, but is it something that has influence on the current PR?

Go ahead!

The relation to sparse matrices is very interesting, but as you say, it's only valid for integer delays.

Mathematically it's the same fractional delays (even with multiple sampling rates).
All our signals live in the same vector space.
(Implementation is a different thing though, that's true.)

I think we should stick with a simple DelayedSignal class, that is a single delay per signal:

  • The data part can always be used "raw" with consistent time alignment.
    Nobody would be forced to use the custom functionality.
  • For the same reason, it's probably a good thing that signals cannot have "holes".
  • When using multiple signals, manual time alignment is IMHO much more tedious than manual weighting. So I don't see why DelayedSignal should care about individual channel weights.

@mgeier mgeier force-pushed the signal-class branch 2 times, most recently from 6a5197c to e7bceef Compare January 11, 2017 13:18
@spors
Copy link
Member

spors commented Feb 10, 2017

We should go ahead with the class and check its application in the toolbox.

@mgeier mgeier merged commit 23cff13 into master Feb 10, 2017
@mgeier
Copy link
Member Author

mgeier commented Feb 10, 2017

OK, I've merged it, thanks for all the comments!

Of course this is not set in stone, we can still change any aspect of it ...

@mgeier mgeier deleted the signal-class branch February 10, 2017 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants