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

Updates for v2.0.0 #150

Merged
merged 99 commits into from
Apr 16, 2019
Merged

Updates for v2.0.0 #150

merged 99 commits into from
Apr 16, 2019

Conversation

TomDonoghue
Copy link
Member

@TomDonoghue TomDonoghue commented Mar 18, 2019

Responds to #140

WARNING: This is an API breaking update. Don't merge until we're all sure we're on board, and also until after we've tagged and released a 1.1.0 release.

Okay - this is getting fairly close (without the test overhaul yet).

A couple open questions:

  • filt uses the name fc for defining filter range, but other modules that call filt, for example time-frequency use the name f_range, which is a bit inconsistent. Should we consolidate on one name?
  • What are the ideas for potentially updating aperiodic sim function names?

Note: not having updated the tests properly yet, I've just turned off a few tests where the old consistency tests became out of data, so confidence in this exact version should be relatively low.

At this stage - perhaps not ready for a proper / formal review, but if people want to skim through - start checking if y'all are on board with the things that are changing, and also if there are things that seemed to be missed so far. Starting trying out using it / stress tests and ideas on testing overhaul also very welcome!

@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Mar 18, 2019
neurodsp/sim/aperiodic.py Outdated Show resolved Hide resolved
Copy link
Contributor

@voytek voytek left a comment

Choose a reason for hiding this comment

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

As I said, I don't have time to review code in detail, but this all broadly looks good.

@TomDonoghue
Copy link
Member Author

@rdgao - do you have thoughts defining filter ranges fc vs f_range (more context above), and/or thoughts suggestions on naming for aperiodic functions?

Also: when you plot a filter kernel, what do the axes represent? What should the labels be?

@TomDonoghue
Copy link
Member Author

Another question - why do we support passing in an array of values as an aperiodic signal into the combined functions? If you already have an aperiodic signal, in an array, then you can just simulate an oscillation and add it to the aperiodic (using our normalized by variance function, if desired).

So - y'all cool with dropping array inputs to combined funcs?

@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Mar 19, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Mar 21, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 10, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 10, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 10, 2019
@rdgao
Copy link
Contributor

rdgao commented Apr 11, 2019

Okay y'all - this is almost it now for the 2.0 version. Or at least, with only a couple open questions w.r.t the newest updates, I don't otherwise really have anything left on my list of things to update.

There are some refactors / updates to sim:

  • Generally, I've refactored the periodic approach to consolidate on specifying and repeating kernels. -
  • This is nice & extendable, but does break a bit with how what is now called sim_bursty_oscillation_features simulates the varying features. If possible, it would be nice to refactor this function to use the different kernels too.
  • There are some quick notes / Qs in the code in sim_powerlaw.
  • Something is a little funky with sim_jittered_oscillation. tbh, I'm not totally clear on what this function is supposed to do.

^ @rdgao : if you can have a check through these things, that would be really useful.

For everything else, we've been broadly reviewing and talking through things as they've gone in. What I suggest then is if this looks about close enough, we merge and make a pre-release, and get people using this version for a bit, to fish out any bugs / problems with it before a proper major release.

okay I just saw there are 110 file changes so let's talk about this in lab tomorrow lol

@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 12, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 12, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 12, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 16, 2019
@TomDonoghue
Copy link
Member Author

Okay @rdgao - I fixed up the main topics from the IRL code review, which is to fix how the aperiodic rotation deals with f=0, and temporarily remove periodic sims that don't work with oscillation kernels / the new approach - I'll open a branch to merge that back in with the new outline.

Otherwise, everything in here has had an overview sign off, and this PR is getting ridiculous, so, now that 1.1.X is released, I'm going to merge this (officially breaking master from 1.0, and moving it to 2.0).

@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 16, 2019
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Apr 16, 2019
@TomDonoghue TomDonoghue changed the title WIP: Updates for v2.0.0 Updates for v2.0.0 Apr 16, 2019
@TomDonoghue TomDonoghue merged commit 9c6f47f into master Apr 16, 2019
@TomDonoghue TomDonoghue deleted the v1 branch April 16, 2019 06:04
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.

None yet

3 participants