OLD / DON'T MERGE - Fix concatenation issues in periodic simulations #189
Conversation
There was a problem hiding this comment.
My understanding of this issue is that edge effects were caused by limiting normalization to non-zero values, which we want for the time inbetween bursts in sim_bursty_oscillation but not sim_oscillation.
I went through all of the code here, and don't completely understand the new tiling and resampling method. It does have an effect on some plots that may not be intentional.
An idea I had after going through the code was to set select_nonzero to False (already default for sim_oscillation but also for sim_bursty_oscillation) and to then normalize on a cycle by cycle basis by moving the normalize decorator to the _make_tilable_cycle func. This wouldn't require the changes to tiling or resampling, that (I think) are causing these differences:
neurodsp/sim/periodic.py
Outdated
| n_cycles = int(np.floor(n_samples / n_samples_cycle)) | ||
| # Here we add a small buffer value to the cycle, so that no values are exactly zero | ||
| # This is because normalization gets applied to non-zero values | ||
| osc_cycle = _make_tilable_cycle(1000, freq, cycle, **cycle_params) + 0.00001 |
There was a problem hiding this comment.
The 0.00001 is so zeros aren't ignore during normalization, right? Could _make_tilable_cycle be decorated to normalize instead of sim_bursty_oscillation? Each cycle should be the same and I think that would work. select_nonzero could then also be set to False.
|
@ryanhammonds - The main issue isn't the normalization, it's the discontinuities noted in the original issue. Those aren't an issue with the normalization, but with concatenation. It does seem that the resampling (or something else) doesn't play nice with some of the cycle types.... I'll have to look into that. |
|
I thought it was normalization since the discontinuities are fixed in master if the normalize decorator is commented from |
THEY ARE!? Well... damn. I think I've been chasing the whole bug this whole time, misunderstanding what's happening. I thought it was something to do with the concatenation, because there were off by one errors or something. So.... the whole resampling thing might just be irrelevant then. Whoooooops. This is actually sorta good news - as the resampling doesn't generalize to different cycle types. My last hacky commit actually drops the resampling anyways. I think I need to come back and check this again, and probably restart a PR, because now this one seems probably wrong-headed. Good catch Ryan! Thanks for figuring out something different here - I'm grateful for the fresh eyes! |
|
so...everything is solved here? also, from Ryan's plot, it seems like the new method introduces a ringing edge artifact, presumably from the antialiasing filter resample automatically applies. Have you guys observed that this is a consistent thing? If so, that might be an issue |
|
@rdgao - the problem is figured out, but this PR is the old / bad version! I'll tag you in to sanity a new & fixed version, but otherwise you can basically ignore this PR. Sidenote: the ringing in Ryan's graph is because the standard scipy Fourier based resampling doesn't play nice with non-sinuosoidal cycles. Luckily, the actual fix seems like it doesn't need any resampling, so I think we can sidestep that whole thing. |
|
okay cool, sounds good, lmk |
TomDonoghue
changed the title
Responds to #174
Updates:
For review:
@rdgao : if you don't mind, since you know this code and we've talked about this before, I'd appreciate if you could throw an eye on the updates and spotcheck for anything you think looks potentially problematic and/or theoretically dubious
@ryanhammonds : please check through as much as you can that the code makes sense, but also for this one, I think a bunch of 'stress testing' is a good idea. Simulate lots of signals, using different types of cycles, and check through them for any issues. Make sure we don't have the discontinuities that were the original issue, and that we haven't introduced some new oddity. You can calculate power spectra on sim'd signals to check that frequencies line up, and that power spectra look as expected, etc.
EDIT: nvm. I think this is the wrong solution (thanks to notes from Ryan!). Pause on this PR, I'm going re-evaluate and likely try again on this.