Skip to content

[ENH] Enable full plot range for fm.plot() #246

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

Conversation

moritz-gerster
Copy link

@moritz-gerster moritz-gerster commented Nov 17, 2022

Fixes #245. With the new functionality, an inspection of fits with a narrow fitting range will be much more user-friendly.

This kind of inspection was recommended in Gerster et al. (2022): Challenge 2: Avoiding Oscillations Crossing Fitting Range Borders.

For new behavior see the last two figures here: Full_Plot_Range.

@TomDonoghue
Copy link
Member

Hey @moritz-gerster - can you merge main branch into your branch? There was an issue with the CI, so I made some maintenance updates to get things back up and running.

@moritz-gerster
Copy link
Author

Hey @moritz-gerster - can you merge main branch into your branch? There was an issue with the CI, so I made some maintenance updates to get things back up and running.

Hey @TomDonoghue, I synced my fork, it should now be up to date!

@moritz-gerster
Copy link
Author

@TomDonoghue let me know if I should change something about the PR such as for example adding some tests 👍

@moritz-gerster
Copy link
Author

Hey @TomDonoghue, I'm happy to add some tests to the PR if you would consider it useful :)

@TomDonoghue TomDonoghue added the idea / discussion A potential idea to consider / discuss. label Jun 28, 2023
@fooof-tools fooof-tools deleted a comment from codecov-commenter Jun 29, 2023
@TomDonoghue
Copy link
Member

Hey @moritz-gerster - thanks for opening a PR on this idea, and sorry for the delay - things have been busy, but we're picking up on some fooof development now.

I added a broader set of notes on this topic in #245, and offered a different strategy in #276. Let me know what you think of that - if it looks good to you, it can replace this, and keep the object more lightweight.

By the way, in terms of implementation, if we were to go this direction (storing the full spectrum in the object), I think that we would actually want to store a single copy of the broad range spectrum, and do something like use a masked array, or do an on-the-fly computation for extracting the fit range, rather than duplicating the arrays.

@moritz-gerster
Copy link
Author

Agreed :)

@TomDonoghue
Copy link
Member

Confirming - following discussion in #245 and #276, a different strategy to allow for the same functionality has now been merged in #276, so closing this PR! Thanks for the work @moritz-gerster - it helped get a cool new feature into the module!

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.

[ENH]: Show full PSD when plotting PSD+model using fm.plot()
2 participants