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

Exclude frequency range of line noise when fitting slope #20

Closed
srcole opened this issue Aug 23, 2017 · 9 comments
Closed

Exclude frequency range of line noise when fitting slope #20

srcole opened this issue Aug 23, 2017 · 9 comments
Labels
1.0 Should go into FOOOF 1.0 release

Comments

@srcole
Copy link
Contributor

srcole commented Aug 23, 2017

I'm fitting a PSD (see below) for a signal after I removed line noise (data available in data.mat here). This biases the slope fit down. It might be a good idea to have a kwarg that allows the user to define frequency range(s) to ignore during the fit.

image

@bringingjoy
Copy link

Sub-range(s) to ignore during full model fitting are now available under the dev branch. Given the potential plural, the optional parameter expects a list of lists (and currently also works with lists of tuples).

@bringingjoy
Copy link

Dev branch is updated with the ignore_range procedure

@TomDonoghue
Copy link
Member

TomDonoghue commented Sep 22, 2017

Let's not close issues until they are merged to master.

This looks great, thanks @bringingjoy !

From playing with it a little:

  • Right now it interpolates in linear space, which makes it curved in log space, which is perhaps not ideal since the assumption is generally that the PSD is linear in log space.
  • Perhaps problematic that it uses interpolated points in slope fitting, and can fit oscillations over 'ignored' regions.

^These things might be moot (unlikely / aren't really relevant) in the use case of excluding small regions, such as line noise, but creep in more if people try to use it to exclude broader ranges, and we should be aware of implications of various ways people might use FOOOF.

I know I suggested this approach, these open issues are totally my bad.

Let me think about it a bit / play around, and see where to go from here.

@bringingjoy
Copy link

I'm sorry about closing the issue preemptively. Don't apologize - I had proposed the linear interpolation, but certainly didn't exhaust testing corner cases, so sorry for the addition of any such potential problems! It's possible that the replaced region's use during slope fitting could negatively impact the resulting fit, but cannot think of a better alternative for this presently - in many use cases, at least, it improves what would otherwise be a poor fit.

As far as fitting "oscillations" introduced by this method, we could exclude oscillations detected on the ignored interval +/- some factor of its bandwidth, or a similar constraint from being added to the overall curve.

Had another idea for accommodating slope fitting with ignoring regions, but could become much less feasible with many ranges specified, so will keep you posted if I think of anything else.

@TomDonoghue TomDonoghue added the v2 label Dec 26, 2017
@rdgao rdgao added enhancement and removed v2 labels Oct 1, 2018
@rdgao
Copy link
Contributor

rdgao commented Oct 1, 2018

I am demanding for this to happen for v1.0!

@parenthetical-e
Copy link

parenthetical-e commented Oct 1, 2018 via email

@TomDonoghue TomDonoghue added the 1.0 Should go into FOOOF 1.0 release label Oct 15, 2018
@TomDonoghue TomDonoghue added v2 and removed 1.0 Should go into FOOOF 1.0 release enhancement labels Jan 20, 2019
@TomDonoghue
Copy link
Member

Bumping to add after v1.

@TomDonoghue
Copy link
Member

TomDonoghue commented Mar 25, 2020

Okay - so I (finally) add a simple solution to a restricted problem space for excluding regions. To limit the scope and avoid some of the problems I think would arise from dealing with large / multiple frequency ranges, and/or dealing with discontinuities, the minimal solution I've added is to interpolate narrow frequency regions in the spectrum itself, prior to fitting. These regions are interpolated as a linear interpolation in log-log space, under the 1/f assumption. This is implemented as a new function interpolate_spectrum, which is now in the utils sub-module. This is done outside the model object, so that it's up to the user to apply this function before model fitting.

I think this solves the general case of line noise (or line noise related bandstop filtered regions), and so deals with the immediate use case. A broader discussion of fitting different ranges or excluding regions is available here: fooof-tools/Development#8

@TomDonoghue TomDonoghue added 1.0 Should go into FOOOF 1.0 release and removed v2 labels Mar 25, 2020
@TomDonoghue
Copy link
Member

Closing this now - future discussion can move to fooof-tools/Development#8, unless anyone wants to re-open with any issues with interpolate_spectrum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Should go into FOOOF 1.0 release
Projects
None yet
Development

No branches or pull requests

6 participants