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

[FIX] - Add fix for special case of rdsym #303

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[FIX] - Add fix for special case of rdsym #303

wants to merge 2 commits into from

Conversation

TomDonoghue
Copy link
Member

For sim_asine_cycle, there are special cases of rdsym value that can lead to an off by one error in terms computing the number of samples in the cycle. This comes from how we compute the number of samples based on rdsym value.

For example, on main, the following code would get the wrong sample length (101 instead of 100):
cyc = sim_asine_cycle2(0.1, 1000, 1., side='both')

This can happen for 'peak' or 'both' when rdsym is 1., and for 'trough' when rdsym is 0.

This PR adds a check and fix for this issue, as well as some extra tests to keep an eye on this issue (note that these tests would fail on current main branch).

Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still an issue when rdsym = .999. There is also another potential bug I found when digging into this. When the trough is asymmetrical, the symmetrical side deviates from expected I think. This isn't an issue when the peak is asymmetrica:

import matplotlib.pyplot as plt
from neurodsp.sim.cycles import sim_asine_cycle

rdsym = .8

plt.plot(sim_asine_cycle(0.1, 1000, rdsym, side='peak'), label='Peak Asym')
plt.plot(sim_asine_cycle(0.1, 1000, rdsym, side='trough'), ls='--',label='Trough Asym')
plt.plot(sim_asine_cycle(0.1, 1000, .5, side='both'), ls='--', label='Sym')
plt.legend()

ex

I think the alternative implementation below fixes the off-by-one samples issue and the side='trough' issue above. It uses scipy's sawtooth to create the phase array instead of stacking linspace arrays.

def sim_asine_cycle2(n_seconds, fs, rdsym, side='both'):

    check_param_range(rdsym, 'rdsym', [0., 1.])
    check_param_options(side, 'side', ['both', 'peak', 'trough'])

    # Determine number of samples
    n_samples = compute_nsamples(n_seconds, fs)
    half_sample = int(n_samples/2)

    # Create phase of asine
    time_asym = np.linspace(0, 2*np.pi, n_samples+1)[:-1] + np.pi * rdsym
    phase = sawtooth(time_asym, rdsym) * np.pi * .5
    
    # Create phase of sine
    if side != 'both':
        time_sym = np.linspace(0, 2*np.pi, n_samples+1)[:-1] + np.pi * .5
        phase_sym = sawtooth(time_sym, .5) * np.pi * .5
        
    if side == 'peak':
        phase[half_sample:] = phase_sym[half_sample:]
    elif side == 'trough':
        phase[:half_sample] = phase_sym[:half_sample]
        
    # Convert phase definition to signal
    cycle = np.sin(phase)

    return cycle

@TomDonoghue TomDonoghue mentioned this pull request Sep 17, 2022
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants