[MRG] Filter Report

[ryanhammonds]

Related to #229 and #139. This PR adds a verbose arg to filter_signal, filter_signal_fir and filter_signal_iir that prints out the filter information (suggested in Widmann et al 2015). This includes:

• Filter type (high-pass, low-pass, band-pass, band-stop, FIR, IIR)
• Cutoff frequency (including definition)
• Filter order (or length)
• Roll-off or transition bandwidth
• Passband ripple and stopband attenuation
• Filter delay (zero-phase, linear-phase, non-linear phase) and causality
• Direction of computation (one-pass forward/reverse, or two-pass forward and reverse)

The previous print_transitions argument functions the same as the verbose arg now (i.e. verbose=np.any([print_transitions, verbose])). I can fully deprecate this arg or modify it to only print transitions.

I'm not sure if the last three bullets are determined correctly (they probably aren't). I'll have to revisit to sort this out.

[TomDonoghue]

Hey @ryanhammonds - what are your thoughts on making this collect the info into a string that can be printed, as you have now, or saved out for later? Something like what FOOOF does for reports?

I'm thinking for the context of a script, or if there was a filter node in NDSPflow. One might want to keep a record of the filter details, but printing to console wouldn't necessarily be the easiest way to do so, and saving out the report might be useful.

What do you think? If this would be super annoying, then it's probably not worth it, but if you think it wouldn't take too long, then it might be useful.

[ryanhammonds]

Hi @TomDonoghue, I've added a save_properties kwarg to filter functions that saves a dictionary of filter properties as a json file. The new kwarg is a str pointing to the name of the json file to be saved. The verbose kwarg can still be used to print out filter properties as well.

[ryanhammonds]

Also, tests are failing here due to an unrelated issue I think - nothing has changed in sim.aperiodic which are failing the tests. I think there was a weird bug in pytest that allowed that test to pass when ran with all of the others tests, but failed when running alone. Updating my version of pytest results in a fail everytime, even on master. I'll make a separate issue/PR adddressing this.

Edit: I fixed the bug in master. a23a192 was needed to pass the travis 'branch' build (since this branch was built on an older master). a23a192 should be dropped when merging since the travis 'Pull Request' build was passing at 6a5695e and this branch will be delete after merge. The changes for a23a192 already exists in master and github doesn't render a diff.

[TomDonoghue]

@ryanhammonds - okay, so I did a review, and left specific comments. Thanks for your work on this - I know this stuff on the nitty-gritty of filter details can be tricky / annoying!

More broadly than the individual comments, I think we might want a slightly different approach here.

Some thoughts:

• I think we should conserve the old function of print_transitions, printing only transitions
  • when using filters on the fly, this info is useful
  • the whole filter report, by comparison, has a lot of useful information, but stuff that I generally know at the moment that I'm using the filter (ex, that I'm using an FIR bandpass), and so is not so useful to print in the moment, but useful to save out to a report for later.
• I think I would vote for the 'report' to be more like what we do in FOOOF, meaning in my mind, collecting the information, and saving out a combined PDF that includes all the filter information and the plots (frequency response & kernel).
• In terms of API, I think perhaps we dont add a 'verbose' argument. For printing info in real time, this can be done with print_transitions and/or plot_properties. I think the added argument, say save_report should collect this text and image output, and save out a PDF.

Or, short version: let's keep print_transitions the same, make the filter report include the plots, and probably drop a separate 'verbose' argument. Thoughts?

If this does sound promising, in terms of creating more of a formatted string to save out in combo with the plots, these might be useful from spectral param:

• formatted strings: https://github.com/fooof-tools/fooof/blob/master/fooof/core/strings.py
• generating reports: https://github.com/fooof-tools/fooof/blob/master/fooof/core/reports.py

[ryanhammonds]

@TomDonoghue I restored the functionality of print_transitions, removed verbose, and added save_report to save out plots and parameters as a pdf. I had to change almost everything in this PR, sorry! I thought we wanted something like the fooof.report() method, and an additional json to store filter "metadata" so it could be replicated easily.

[ryanhammonds]

I met with Brad yesterday to discuss some of the things I am working on and he voted to keep verbose here, in addition to save_report. It can exist without conflicting with print_transitions or save_report. I'll push this back in soon.

[TomDonoghue]

So what will 'verbose' do then? It's not clear to me it adds any functionality...?

[ryanhammonds]

It prints the full filter report to console without having to open the pdf:

Edit: I guess 3/11 reported params are in the func call but the 8 other could be useful.

[TomDonoghue]

So yeh, in terms of what verbose would print out the informative info is available in print_transition. Things like "FIR" or (8-12) are available from the API call, and even info like "1-pass" are inherent in the filter called (will never change with different calls). If verbose is a shortcut for print_transition + plot_properties that seems... okay? But like, feels a bit like API clutter for not much benefit, and with being verbose being such a common arg - I think it's not obvious that it would print plots.

Anyways - sorry for the confusion on this one - referring back to your previous comment, I also have in mind that this should be like fooof.report() in terms of making that structured report - though I guess the difference is that in case it's not as much about printing in real time, but creating the report to save out.

So - what still needs doing here?

[ryanhammonds]

So the plots are only plotted in my example because plot_properties is passed. It's unrelated to verbose, which only includes what is printed in the output similar to standard verbose args. There was a lot printed with verbose arg that isn't obvious (at least to me) and thought it may be useful.

Not a big deal to me either way so no need to go on about this. Verbose has been dropped already. With the exception of the one unresolved comment, it should be how you wanted and ready to merge. Feel free to direct push any changes you want or to close.