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

Added goodness of fit fields to LsqFitResult #59

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@MLackner

MLackner commented Nov 17, 2017

Namely the coefficient of determination and the standard error of the
estimate

Added goodness of fit fields to LsqFitResult
Namely the coefficient of determination and the standard error of the
estimate
@IlyaOrson

IlyaOrson suggested changes Dec 5, 2017 edited

Thanks for this! Maybe you could make clear in the README that the weights [wt] are usually the reciprocals of the errors: wt = 1/error. Cheers!

Show outdated Hide outdated src/curve_fit.jl
Show outdated Hide outdated src/curve_fit.jl
Show outdated Hide outdated src/curve_fit.jl
jacobian::Matrix{T}
converged::Bool
wt::Array{T,N}
end
# provide a method for those who have their own Jacobian function
function lmfit(f::Function, g::Function, p0, wt; kwargs...)
function lmfit(model::Function, xpts::AbstractArray, ydata::AbstractArray, f::Function, g::Function, p0, wt; kwargs...)

This comment has been minimized.

@pkofod

pkofod Dec 5, 2017

Collaborator

can you explain what this is? people are supposed to pass a model and f and g?

@pkofod

pkofod Dec 5, 2017

Collaborator

can you explain what this is? people are supposed to pass a model and f and g?

This comment has been minimized.

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

I also am confused about why you need model and f.

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

I also am confused about why you need model and f.

@pkofod

This comment has been minimized.

Show comment
Hide comment
@pkofod

pkofod Dec 5, 2017

Collaborator

a few comments... don't be afraid to bump, I might look at the PR and then forget a few days after!

Collaborator

pkofod commented Dec 5, 2017

a few comments... don't be afraid to bump, I might look at the PR and then forget a few days after!

@halleysfifthinc

This comment has been minimized.

Show comment
Hide comment
@halleysfifthinc

halleysfifthinc Feb 25, 2018

@MLackner @pkofod I'd like to see this get merged so I can begin using the rsquared. Is there anything I can do to help this along?

halleysfifthinc commented Feb 25, 2018

@MLackner @pkofod I'd like to see this get merged so I can begin using the rsquared. Is there anything I can do to help this along?

@pkofod

This comment has been minimized.

Show comment
Hide comment
@pkofod

pkofod Feb 25, 2018

Collaborator

Well, if @MLackner returns we can hopefully merge it soon, if not we could try to take his suggestions and start a new PR.

Collaborator

pkofod commented Feb 25, 2018

Well, if @MLackner returns we can hopefully merge it soon, if not we could try to take his suggestions and start a new PR.

@pkofod

This comment has been minimized.

Show comment
Hide comment
@pkofod

pkofod Mar 21, 2018

Collaborator

Seems like OP moved on. We should try to create a parallel PR. It's up for grabs, but @iewaij might be interested? Between myself, @halleysfifthinc, @IlyaOrson and @iewaij we're four people. That should be sufficient to add goodness of fit relatively soon ;)

Collaborator

pkofod commented Mar 21, 2018

Seems like OP moved on. We should try to create a parallel PR. It's up for grabs, but @iewaij might be interested? Between myself, @halleysfifthinc, @IlyaOrson and @iewaij we're four people. That should be sufficient to add goodness of fit relatively soon ;)

@MLackner

This comment has been minimized.

Show comment
Hide comment
@MLackner

MLackner Mar 21, 2018

Oh hey!
Sorry, I lost track of this issue. I'll find time today working on this PR.

MLackner commented Mar 21, 2018

Oh hey!
Sorry, I lost track of this issue. I'll find time today working on this PR.

@pkofod

This comment has been minimized.

Show comment
Hide comment
@pkofod

pkofod Mar 21, 2018

Collaborator

Oh hey!
Sorry, I lost track of this issue. I'll find time today working on this PR.

No worries! Promise to keep it alive on my side of the table

Collaborator

pkofod commented Mar 21, 2018

Oh hey!
Sorry, I lost track of this issue. I'll find time today working on this PR.

No worries! Promise to keep it alive on my side of the table

@MLackner

This comment has been minimized.

Show comment
Hide comment
@MLackner

MLackner Mar 21, 2018

This is certainly not the most elegant way I did it but it works. I compared the goodness of fit results with the results Matlab spits out and I got the same values for sse and rmse but I had differences for r-squared and adj-r-squared at the third decimal place which I can't explain.

MLackner commented Mar 21, 2018

This is certainly not the most elegant way I did it but it works. I compared the goodness of fit results with the results Matlab spits out and I got the same values for sse and rmse but I had differences for r-squared and adj-r-squared at the third decimal place which I can't explain.

@pkofod

This comment has been minimized.

Show comment
Hide comment
@pkofod

pkofod Mar 21, 2018

Collaborator

adj-r-squared at the third decimal place which I can't explain.

are the estimates exactly the same?

Collaborator

pkofod commented Mar 21, 2018

adj-r-squared at the third decimal place which I can't explain.

are the estimates exactly the same?

@MLackner

This comment has been minimized.

Show comment
Hide comment
@MLackner

MLackner Mar 22, 2018

adj-r-squared at the third decimal place which I can't explain.

are the estimates exactly the same?

They differ at the eleventh decimal place.

MLackner commented Mar 22, 2018

adj-r-squared at the third decimal place which I can't explain.

are the estimates exactly the same?

They differ at the eleventh decimal place.

@blakejohnson

The parameters you are calculating are only statistically valid for linear regressions.

# fit.dof: degrees of freedom
# fit.param: best fit parameters
# fit.rsquared: coefficient of determination
# fit.stderror: standard error of the estimate

This comment has been minimized.

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

This quantity is already available via standard_error(fit).

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

This quantity is already available via standard_error(fit).

# fit.dof: degrees of freedom
# fit.param: best fit parameters
# fit.rsquared: coefficient of determination

This comment has been minimized.

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

R squared is meaningless for a nonlinear regression

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

R squared is meaningless for a nonlinear regression

@@ -1,23 +1,61 @@
immutable LsqFitResult{T,N}
struct GoodnessOfFit{T}
sse::T

This comment has been minimized.

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

Equivalent to the resid already present in LsqFitResult

@blakejohnson

blakejohnson Jul 18, 2018

Contributor

Equivalent to the resid already present in LsqFitResult

@iewaij

This comment has been minimized.

Show comment
Hide comment
@iewaij

iewaij Jul 18, 2018

Contributor

Maybe we can introduce a curve_fit_tools.jl? It could provide similar functions in this PR and other utility functions like Base.show(), curve_plot() and bootstrap_covar(). I've been working on this, see here.

Contributor

iewaij commented Jul 18, 2018

Maybe we can introduce a curve_fit_tools.jl? It could provide similar functions in this PR and other utility functions like Base.show(), curve_plot() and bootstrap_covar(). I've been working on this, see here.

@halleysfifthinc

This comment has been minimized.

Show comment
Hide comment
@halleysfifthinc

halleysfifthinc Jul 18, 2018

FWIW, I don't think baking this into LsqFit is necessary and/or a good idea anymore. Given that the coefficient of determination is not the same thing between a linear vs. nonlinear regression, I think this is something that should be left up to the user. At the very least, I think a "lazy" calculation would be better (ie a function call to calculate the coefficient) as not all users will want or need this.

I implemented this for myself with a simple:

rsquared = cor(data,model(t,fit.param))^2

which is correct for a linear regression (and simpler IIRC than the changes here).

halleysfifthinc commented Jul 18, 2018

FWIW, I don't think baking this into LsqFit is necessary and/or a good idea anymore. Given that the coefficient of determination is not the same thing between a linear vs. nonlinear regression, I think this is something that should be left up to the user. At the very least, I think a "lazy" calculation would be better (ie a function call to calculate the coefficient) as not all users will want or need this.

I implemented this for myself with a simple:

rsquared = cor(data,model(t,fit.param))^2

which is correct for a linear regression (and simpler IIRC than the changes here).

@pkofod

This comment has been minimized.

Show comment
Hide comment
@pkofod

pkofod Oct 11, 2018

Collaborator

I don't think we should allow users to calculate r2 easily, since it's non invalid under non-linearity in coefficients. The remaining changes are now in master some way or the other (or were already there at the time up the last update). Thanks for the work anyway @MLackner, it's appreciated that people interact and contribute. You're more than welcome to tackle any of the remaning issues with this package, contribute examples or just use the software if it helps you do your work.

Nonetheless, I'm closing this PR in favor of existing commits that have been merged into master already.

Collaborator

pkofod commented Oct 11, 2018

I don't think we should allow users to calculate r2 easily, since it's non invalid under non-linearity in coefficients. The remaining changes are now in master some way or the other (or were already there at the time up the last update). Thanks for the work anyway @MLackner, it's appreciated that people interact and contribute. You're more than welcome to tackle any of the remaning issues with this package, contribute examples or just use the software if it helps you do your work.

Nonetheless, I'm closing this PR in favor of existing commits that have been merged into master already.

@pkofod pkofod closed this Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment