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

Implement fstatistic #424

Merged
merged 1 commit into from Nov 2, 2021
Merged

Implement fstatistic #424

merged 1 commit into from Nov 2, 2021

Conversation

OndrejSlamecka
Copy link
Contributor

@OndrejSlamecka OndrejSlamecka commented Apr 25, 2021

To be used by JuliaStats/StatsModels.jl#225

I am not sure how much sense does nobs::Float64 make but Float64 is the return type of nobs.

src/linpred.jl Outdated Show resolved Hide resolved
src/linpred.jl Outdated Show resolved Hide resolved
src/lm.jl Outdated Show resolved Hide resolved
src/lm.jl Outdated Show resolved Hide resolved
src/lm.jl Outdated Show resolved Hide resolved
src/lm.jl Outdated Show resolved Hide resolved
src/lm.jl Outdated Show resolved Hide resolved
@OndrejSlamecka
Copy link
Contributor Author

Thank you @palday for the suggestions, I implemented them, except un-deleting the two modelframe related functions for which I commented above. Is it desirable to also export fstatistic in GLM.jl?

@palday
Copy link
Member

palday commented Apr 26, 2021

Thank you @palday for the suggestions, I implemented them, except un-deleting the two modelframe related functions for which I commented above. Is it desirable to also export fstatistic in GLM.jl?

I only focused on some Julia/style issues; I'll let @nalimilan or @kleinschmidt comment on API choices. 😄

@samusz
Copy link

samusz commented Sep 1, 2021

Seems related to issue #400 , if I am not mistaken.

@nalimilan
Copy link
Member

Sorry for not commenting earlier @OndrejSlamecka. How about making this a method of ftest? Since the p-value is printed, this is not just a function to get the F-statistic. That also simplifies the API.

@OndrejSlamecka
Copy link
Contributor Author

OndrejSlamecka commented Sep 4, 2021

Re "making this a method of ftest": so in

function ftest(mods::LinearModel...; atol::Real=0.0)
turn, for example the following one into the one below? (To be clear, I just made up 42 and 0.42.)

julia> ftest(nullmodel.model, model.model)
F-test: 2 models fitted on 12 observations
─────────────────────────────────────────────────────────────────
     DOF  ΔDOF     SSR     ΔSSR       R²     ΔR²        F*  p(>F)
─────────────────────────────────────────────────────────────────
[1]    2        3.2292           -0.0000                         
[2]    3     1  0.1283  -3.1008   0.9603  0.9603  241.6234  <1e-7
─────────────────────────────────────────────────────────────────
julia> ftest(nullmodel.model, model.model)
F-test: 2 models fitted on 12 observations
─────────────────────────────────────────────────────────────────
     DOF  ΔDOF     SSR     ΔSSR       R²     ΔR²        F*  p(>F)
─────────────────────────────────────────────────────────────────
[1]    2        3.2292           -0.0000              42.0   0.42      
[2]    3     1  0.1283  -3.1008   0.9603  0.9603  241.6234  <1e-7
─────────────────────────────────────────────────────────────────

That'd be nice API-wise. A disadvantage is that it wouldn't show when viewing the model summary (JuliaStats/StatsModels.jl#225) which is something to consider.

If you want to go ahead with the "extending ftest" approach, I'll update the code and the comment (I reckon it's a matter of dropping/replacing the two NaNs here

fstat = (NaN, (MSR1 ./ MSR2)...)
).


I also just rebased on latest master.

@nalimilan
Copy link
Member

Adding this information to the first row of the table could be confusing, as it doesn't have the same meaning as in other rows. I was just thinking about using the ftest function with a single argument. The returned object could be a different type from FTestResult, similar to your FStatistic type (but with a name like SingleFTestResult or FTest1Result). Can you also put the new code in ftest.jl?

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021

Codecov Report

Merging #424 (06ba845) into master (96f6eeb) will increase coverage by 0.44%.
The diff coverage is 100.00%.

❗ Current head 06ba845 differs from pull request most recent head 795a0df. Consider uploading reports for the commit 795a0df to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   83.39%   83.84%   +0.44%     
==========================================
  Files           6        6              
  Lines         771      786      +15     
==========================================
+ Hits          643      659      +16     
+ Misses        128      127       -1     
Impacted Files Coverage Δ
src/ftest.jl 100.00% <100.00%> (+1.47%) ⬆️
src/linpred.jl 76.57% <100.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96f6eeb...795a0df. Read the comment docs.

@OndrejSlamecka
Copy link
Contributor Author

@nalimilan I've done that now. Please let me know what you think.

Project.toml Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
src/ftest.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/linpred.jl Outdated Show resolved Hide resolved
src/GLM.jl Show resolved Hide resolved
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks! This is something we could consider moving to StatsModels so that other packages don't have to reimplement it. Would probably be worth filing an issue there to discuss it, if you feel like it.

@nalimilan nalimilan merged commit 4e72e76 into JuliaStats:master Nov 2, 2021
ym-han pushed a commit to ym-han/GLM.jl that referenced this pull request Dec 17, 2021
@palday palday mentioned this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants