[ENH] Burst and feature refactor duplicate.

[ryanhammonds]

I accidentally merge #61 into master, and it closed before review, without the possibility of reopening. I rolled the merged commits back so these commits could get a proper review before merging. Here is the original post:

This PR is related to #32 and #46. bycycle.features has been broken down into:

• bycycle.features.shape: computes cycle shape features (period, time_decay, volt_rise, etc)
• bycycle.features.burst: computes cycle burst features (monotonicity, amplitude_fraction, etc)
• bycycle.features.features: wraps these two functions to compute all features.

These functions may optionally return df_samples, which contains samples/signal indices for where cyclepoints occur. This should be the metadata that Erik's comment referred too.

Regarding #46, bycycle.burst now serves only to detect where a signal is oscillating, and to append the is_burst column to the feature dataframe. It doesn't require the signal as an arg (only needs the features dataframe) and won't require all features to be recomputed.

Here is an example, compute_features functions the same with exception of optionally returning a samples dataframe:

# Determine cycles that are bursting add an 'is_osc' column.
threshold_kwargs = {'amp_fraction_threshold': .5,
                    'amp_consistency_threshold': .5,
                    'period_consistency_threshold': .5,
                    'monotonicity_threshold': .5,
                    'n_cycles_min': 3}

# Compute both shape and burst features, and return cyclepoint sample indices in a separate df
df_features, df_samples = compute_features(sig, fs, f_range,  burst_method='cycles' \
            threshold_kwargs=threshold_kwargs, return_samples=True)

Tests have been updated. I'm holding off on updating the tutorials/examples until the code here has been finalized. I can add that to a separate PR to try to keep this PR smaller than the plotting refactor.

Also here is a list of variable names that have been renamed from 0.1.2:

# We decided on this since 'peaks' wasn't 'extrema_peaks'
zerox_rise -> rises
zerox_decay -> decays

# Consistency with neurodsp
x -> sig
Fs -> fs
N_cycles_min -> min_n_cycles

# Increased modularity
df -> df_samples, df_features

# Consistency between burst detection kwargs and dataframe column names
amplitude_fraction_threshold -> amp_fraction_threshold
amplitude_consistency_threshold -> amp_consistency_threshold

# Matching plotting kwargs with compute_features kwargs
osc_kwargs -> burst_kwargs

# pep8
Ps -> ps
Ts -> ts
zeroriseN -> rise_xs
zerofallN -> decay_xs
hilbert_increase_N -> deprecated

# other
_fzerorise -> find_flank_zerox
_fzerofall -> find_flank_zerox

[TomDonoghue]

Okay, this one has a slightly dis-jointed review again.

I looked through mostly on a local copy of code, so a lot of small fixes and so on, I just edited as I went. You can check these in my 3rd commit. I dug in a bit more on the cyclepoints file, playing with some refactors. Please sanity check these in the 2nd commit. And then, overall, I collected some thoughts and suggestions and left individual review comments on leftover things.

So far I've focused on what comes to mind as I checked file by file, etc. At some point before the full 1.0 release, we'll want to check (maybe just chat through) the overall workflow and logic, and make sure it all makes sense.

So far though, this all looks really nice! Thanks for the work here!

[TomDonoghue]

I've had another check through the code. All the updates look great!

I did another sweep through the code, and made some small lint related updates and doc fixes, etc. And then I was playing with organizing

Things I updated:

• consolidated on the parameter name min_n_cycles (it varied a little in different places)
• drop access to hilbert_increase_n (see: https://github.com/neurodsp-tools/neurodsp/issues/214)
• re-org individual files into sub-folders, and related re-orgs for this.
  • This was done partly for a hopefully clear conceptual layout, and to be forward looking (not everything needed spitting out now, but hopefully this is a good layout to develop on).
  • Note that generally access to functions (how they get imported) didn't change much, because most things are accessible through __init__. The organization is more internal than for the user.

So, @ryanhammonds I'm going to throw it back to you to review the updates. Some tasks:

• Review the updates. Do my tweaks make sense, and what do you think of the updated organizations?
• I didn't properly update the tests. Can you update the test organization to match the folder structure and file layout, etc.
• Some values, such as in detect_bursts_cycles have ranges that the values should take, but we never check them. Can you add a checker for this? It could be modelled on what we added to NeuroDSP in here (https://github.com/neurodsp-tools/neurodsp/pull/210)
• Can you update your original comment in this thread, to be up to date with the new names & API, etc? This will help for things like the Changelog later.

After these updates, I think we can do one final check here, and then hopefully we are done on the main refactor here!

[ryanhammonds]

Hi @TomDonoghue, your updates and restructure make sense to me. The one refactor that maybe could be renamed is bycycle.cyclepoints.extrema to bycycle.cyclepoints.cyclepoints, since this sub-module includes zero-crossings in addition to extrema and a similar naming was kept in bycycle.features.features. I'm not sure if naming a folder and submodule the same is normal. If not, maybe both could be renamed. Other than that, everything looks great!

I push commits to restructure tests and to add argument checking anywhere where an expected range is known. I think this PR is ready to merge.

[TomDonoghue]

Okay @ryanhammonds - I think we are there!

I made some mild updates:

• checking & organizing tests a bit. for folders of tests, adding __init__ avoids issues of having test files with the same names.
• updating naming within cyclepoints. since there are two different kinds of tasks, i split up into extrema & zerox.
• update a little checking. Basically, I recommend we check a value in the function where it's defined / used - otherwise we can end up checking the same value multiple times, which is also harder to maintain. Related to this, I removed some checks, like for f_range, which gets checked when it's passed through to the filter function
• general lints & words stuff

[TomDonoghue]

For the sake of moving on, and our sanity with this monstrous PR - I'm going to merge this in now.

Any leftover nitpicks with this refactor can be addressed in subsequent updates, as we finish 1.0.

Thanks for a ton of work on this one @ryanhammonds!