Possible API Changes & Refactoring

[TomDonoghue]

The following is a set of questions / suggestions for some API breaking updates, if we want to do that (presumably, in a 2.0 release or similar) and as well as some possible refactoring ideas. (This is just a dump of some things I noted down randomly, and am dumping here).

Filter:

• after the refactor, everything currently maintains the old API, but there are some names that are not great & parameter ordering could be cleaner.
• We can update the plot_frequency_response to either change it's inputs, or at least directly use the filter function to compute_frequency_response (drop duplicate code).

Sim:

• Why do we use the word 'oscillator'? I find it a little confusing - oscillator implies a generator / generative process, but really were simulating oscillations / rhythms right? Can we change this?
• Similarly, maybe we want to move away from the name 'noise'? I know some of the things we generate are 'statistical noise', but for consistency it might be nice to consolidate (across tools), on talking about periodic & aperiodic components.
• Sim is large and likely to grow. We could consider making this a folder with 'sim_periodic', 'sim_aperiodic' and 'sim_combined' or similar.
• Why does sim_filtered_noise make it's own filter, instead of using ndsp.filter?

Spectral:

• The rotate function can be refactored, as in FOOOF, and maybe even moved to live with sim, which is where it's used. It has some funky param names that could / should change (delta_f -> delta_exponent, or similar).
• There is some refactoring that would clean things up a little, for example, separating out a helper function for dealing with nperseg, that is currently a lot of duplicated code.
• There are different names and conventions used for outlier removal -> consolidate on same API

Time Frequency:

• _get_filter_passtype feels like it could be moved to filter, as a helper function called infer_passtype?
• remove_edge_artifacts could be split out as it's own function (copied code), or rather, use the same helper function that is defined in filter.

Utils [New]:

• There are a bunch of helper / utility functions that overlap between modules (such as dealing with NaNs, dropping edges, etc), that could be split out into their own utilities file.

Tests:

• Consolidate on a smoke test procedure, that generates data on the fly and tests for execution and as much sanity checking and accuracy testing as we can - drop test data files.

In terms of action points: this is all pretty easy to do, if it's wanted. There is a quick discussion to be had, in particular, about the naming conventions for sim I think. Most of the rest is very straight-forward, it's just about if we do want some API breaking changes in a new release. Thoughts @voytek @rdgao @srcole

[voytek]

These are all good suggestions. I think it warrants some conversation in person (lab meeting or otherwise) about what names to choose for things, but otherwise I appreciate the effort to make things make more sense and be more consistent :)

[rdgao]

^ ditto - good call. I'm onboard with most of those.

• not sure why sim_filtered_noise has its own filter, no reason to not replace with ours.
• rotate_spectrum should be made consistent with the version in fooof, since that's the most clean one, if we decide to still keep this as a separate function
• super agree on changing oscillator to oscillation or rhythm
• the only point I disagree on is changing noise to aperiodic: as of right now, all the non-oscillation signals are statistical noise, which is a strict subset of "aperiodic". I'm game for having separate folders for sim_periodic and sim_aperiodic, but (especially) if we were to include other simulators like ERPs/non-rhythmic transients, it's actually important to maintain the noise names to distinguish them from non-rhythmic but non-noise aperiodic signals. I agree that we should make it consistent across the tools, but fooof does not distinguish between the types of aperiodic activity, whereas the forward models in sim could.

[TomDonoghue]

I totally agree that we should have transients at some point, though I could see this working:

sim/periodic
sim/aperiodic
sim/transients

^So I'm not sure having transients, or other sims, is a roadblock here.

I'm not against aperiodic being called noise, but I think it's a balance if it's worth being more technically correct (in terms of statistical noise), even if that does bring some baggage of using that term. Also, is something like the OU process appropriately described as statistical noise? The more general name might be less likely to have exceptions, especially if we add things.

Anyways - yeh: I think an IRL discussion would be useful here. Not sure it's so general as to be a lab meeting thing, but we could have a quick prog group meeting.

[rdgao]

so the takeaway from that discussion was we organize the code in folders as you have them above, but keep the names as descriptive and accurate as possible, right?

[TomDonoghue]

Yeh - the file names will be as in #147 (unless there are suggestions for otherwise). Currently these are set up as non-breaking changes, and all functions are aliased to sim/.

For a 1.0 release we will update some function names, and all those will be updated to be as accurately as possible. Some of those names will require case-by-case discussion, as well as some broad overview to try and keep a consistent vocabulary overall.

Note: perhaps we can choose & update those names in lockstep with updating the glossary (#144) so that the words we use and the ways in which we are using them gets built into the docs as we go.

[TomDonoghue]

Okay, so everything in here is basically done, and we've moved on from these ideas. These changes are all ready in #150 , and the up to date conversation is happening there, so I'm going to close this now.