Fm vco #61
Fm vco #61
Conversation
Codecov Report
@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 98.87% 98.97% +0.09%
==========================================
Files 9 9
Lines 714 779 +65
==========================================
+ Hits 706 771 +65
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| def make_control_as_frequency(self, mod_signal: np.ndarray): | ||
| modulation = self.p("mod_depth") * mod_signal | ||
| control_as_midi = self.p("pitch") + modulation | ||
| return midi_to_hz(control_as_midi) | ||
|
|
There was a problem hiding this comment.
Factoring this section, as this is what FmVCO has to replace. Could be a better name for this method.
Effectively we're pivoting between various representations of instructions: (1) midi values, (2) instantaneous frequency (Hz), and (3) phase, which ultimately controls the oscillator.
There was a problem hiding this comment.
pitch_signal_to_frequency? I think what you have is also okay
| @@ -368,16 +368,21 @@ def _npyforward(self, mod_signal: np.array, phase: float = 0.0) -> np.ndarray: | |||
|
|
|||
There was a problem hiding this comment.
hmmmm so changing numpy functionality that has already been ported to torch (VCO), you should also port these changes to torch.
| @@ -368,16 +368,21 @@ def _npyforward(self, mod_signal: np.array, phase: float = 0.0) -> np.ndarray: | |||
|
|
|||
| """ | |||
|
|
|||
| assert (mod_signal >= 0).all() and (mod_signal <= 1).all() | |||
| assert (mod_signal >= -1).all() and (mod_signal <= 1).all() | |||
There was a problem hiding this comment.
Can you explain why we need this change? Is it only FmVCO where mod_signal >= -1 and for all the others its >= 0?
There was a problem hiding this comment.
Frequency modulation adjusts the frequency both above and below the central frequency; that's how the theory works and how you get the nice central pitch percept.
But I'm thinking that mod's should be -1 < mod < 1 in general though. Makes for a more flexible synth. E.g., this is how we'd want to implement an LFO.
| ): | ||
|
|
||
| super().__init__(midi_f0=midi_f0, mod_depth=mod_depth, phase=phase) | ||
|
|
||
| def oscillator(self, argument): | ||
| return np.cos(argument) | ||
|
|
||
|
|
| def _forward(self, control_in: T, audio_in: T) -> T: | ||
| assert (control_in >= 0).all() and (control_in <= 1).all() | ||
|
|
||
| if (audio_in <= -1).any() or (audio_in >= 1).any(): | ||
| normalize(audio_in) | ||
|
|
||
| audio_in = fix_length(audio_in, len(control_in)) | ||
| return control_in * audio_in |
There was a problem hiding this comment.
This might be controversial, but needed to be addressed in both the npy and torch modules:
Before the VCA clipped both control and audio signals. Here, we assert the control is in the right range (unlike pitch mod, no reason for it to ever be negative; and no reason to be above 1). The audio is generally well-behaved, but sometimes can creep outside [-1, 1]. So, we normalize it.
With most random synths this is not the case, but it's here as a fail safe. It shouldn't effect synth performance.
|
This pull request introduces 1 alert when merging b4a2ace into 9a24cfa - view on LGTM.com new alerts:
|
|
Cool great :) Do you want to add tests? If you're having trouble I can help |
|
This pull request introduces 1 alert when merging c48d88a into 9a24cfa - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 9252fd9 into 9a24cfa - view on LGTM.com new alerts:
|
Made
FmVCOclass, which basically has to process the modulation signal in a slightly different way. Typically the mod signal is applied in midi-space (log frequency), but FM operates on modulations in Hz space. Also changed the modulation depth to reflect the classic 'modulation index' of FM literature. It's a bit more intuitive this way.Had to slightly refactor
VCOto make this smooth.Note, this is only work on the numpy modules.
torchmodule.pywould have to be updated accordingly.