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

add r-squared metric #252

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

Conversation

mwprestonjr
Copy link
Contributor

R-squared metric added to FOOOF and FOOOFGroup objects, and FOOOFGroup report plot.
R-squared is adjusted for the number of parameters in the model, facilitating model comparison (#251).

Add r-squared metric to FOOOF and FOOOFGroup objects, and FOOOFGroup
report plot.
R-squared is adjusted for the number of parameters in the model,
facilitating model comparison.
@TomDonoghue
Copy link
Member

Hey @mwprestonjr - thanks, this looks great!

Is it okay with you if I target this for the 2.0 release, which will have the breaking change of name and so on? Since this updates the set of attributes on our core objects, I think it makes sense to put this update there. If this sounds good to you, I'll review this PR here, then redirect it to the 2.0 branch, and merge it there to be part of that release.

@mwprestonjr
Copy link
Contributor Author

Yea that sounds great, thanks.

@voytek voytek added the 2.0 Targetted for the specparam 2.0 release. label Jun 28, 2023

# compute adjusted r-squared
n = len(self.power_spectrum) # number of data points
k = len(self.peak_params_) * 3 + 2 # number of parameters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwprestonjr - just checking in here: the number of parameters here is computed as n_peaks * 3 (reflecting the 3 Gaussian params per Gaussian), and then + 2, presumably for aperiodic - but I think this should be +2 if fixed, +3 if knee mode, right? Or, more generally, n_params = n_peaks * n_params_per_peak + n_ap_param?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Tom, nice catch. Sorry I missed this. I've now made this update.

@TomDonoghue
Copy link
Member

Hey @mwprestonjr - I'm keeping this in mind for integrating into 2.0, which I plan to include a somewhat broader framework for specifying and calculating goodness of fit measures. I left a quick comment above to make sure I understand.

I'm still working on some re-organizations for 2.0 that will add the new fit measures, and this will need a bit of refactoring then - do you want me to tag you back in when this PR could be refactored into 2.0, or is it easier if I go ahead and adapt this PR into some of the updates directly?

parameter count has been updated to take into account the aperiodic
mode.
@mwprestonjr
Copy link
Contributor Author

Hey @TomDonoghue, sorry for the late reply. It's likely easier for you to adapt this PR into some of your updates directly at this point. But let me know what you think. Happy to help

@fooof-tools fooof-tools deleted a comment from codecov-commenter Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 Targetted for the specparam 2.0 release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants