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

[ENH] - Add one sided asymmetric option to asine cycle #254

Merged
merged 4 commits into from
Apr 7, 2021
Merged

Conversation

TomDonoghue
Copy link
Member

@TomDonoghue TomDonoghue commented Mar 1, 2021

This update adds support for creating an asymmetric cycle with only one asymmetric extrema, to sim_asine_cycle.

For example, this code:

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

fs, freq = 1000, 10
cycle = sim_asine_cycle(1/freq, fs, 0.25, 'peak')

plt.plot(cycle)

Would now work, to have a 'peak-only asymmetric cycle', and gives this:
Screen Shot 2021-03-01 at 8 42 46 AM

As a limitation of this - it seems unideal that there is an abrupt shift from the asymmetric to symmetric regime at the zero crossing point (ideally this might be smoother), but broadly I think this should be okay, and this update can still be useful for some things.

Edit: I've been playing around with those code for awhile, including poking at different ways to do the offsets, and if there's a way to refactor what looks to be duplicated code. At this point, I think it's probably easiest to stick with it as is, but one way or another I'm moving this to review stage to get some more eyes on it!

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.

Everything looks good here in terms of functionality. I tested this out in a not book but am still wrapping my head around how the various phase arrays work.

@TomDonoghue
Copy link
Member Author

Thanks for checking @ryanhammonds! I don't think anything necessarily needs any additional figuring out here, so I'm going to call it, and merge this one in!

@TomDonoghue TomDonoghue merged commit f6accd0 into main Apr 7, 2021
@TomDonoghue TomDonoghue deleted the half_asym branch April 7, 2021 02:04
@ryanhammonds ryanhammonds mentioned this pull request May 3, 2021
26 tasks
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.

2 participants