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

Adds more flexibility in the aperiodic/oscillatory fitting #147

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

metapfhor
Copy link

@metapfhor metapfhor commented Aug 22, 2019

DESCRIPTION:
I have added a keyword ap_range to the FOOOF.fit() function.

When ap_range is specified, the already existing freq_range keyword
is used to determine the frequencies for the oscillatory fits while the
ap_range keyword is used for the aperiodic fit. When it is not
specified, everything proceeds as before.

This allows the two parts FO and OOF to be more-or-less independent of
one another if so-desired.

Furthermore, the ap_range keyword may also be a set of indices in
order to allow for a very simple implementation of exclusion zones.

Update: I have added support for these keyword options in the FOOOFGroup.fit() function.

TESTING:
Tested on ~100 different EEG files, no errors unless inputs are the wrong
size.
Screenshot_2019-08-22_16-22-46
Screenshot_2019-08-22_16-05-29

DESCRIPTION:
I have added a keyword `ap_range` to the FOOOF.fit() function.

When `ap_range` is specified, the already existing `freq_range` keyword
is used to determine the frequencies for the oscillatory fits while the
ap_range keyword is used for the aperiodic fit. When it is not
specified, everything proceeds as before.

This allows the two parts FO and OOF to be more-or-less independent of
one another if so-desired.

Furthermore, the  `ap_range` keyword may also be a set of indices in
order to allow for a very simple implementation of exclusion zones.

TESTING:
Tested on 5 different EEG files, no errors unless inputs are the wrong
size.

Update: I have also modified the FOOOFGroup.fit() in order to work with
this keyword.
@TomDonoghue
Copy link
Member

Hey @metapfhor:
Sorry for such a belated response to this! Thanks a lot for the suggestion / update - I do think this is a very interesting idea.

I've recently come back around to a series of code updates, and been thinking about this a bit more. I think there would be a couple things I would want to check before this would be ready to merge in:

  • there are some criteria we use to deal with peaks at the edges, and it might be worth double checking how these are applied given the different frequency ranges
  • freq_range is used a bunch, including through saving & loading, to regenerate data and models, and I would want to check through nothing goes weird here, with the update
  • I would want to think about recommendations for when to do this, as typically I think it's often important to fit both components together
  • Note that related to simple exclusion zones, I've added an interpolation function to remove things like line noise peaks (Exclude frequency range of line noise when fitting slope #20)

By the way, I'm not asking for you to jump into any doing / checking of these, just logging some notes on it! We've moved development questions and coordination to a new repo (linked below). My current thought is to leave this until after we do the release of the current 1.0 candidate, and then hopefully I'll have time to come back around and work through this properly.

Related development issue: fooof-tools/Development#8

Base automatically changed from master to main January 26, 2021 19:12
@TomDonoghue TomDonoghue added the idea / discussion A potential idea to consider / discuss. label Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea / discussion A potential idea to consider / discuss.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants