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

Get rid of Tableregressionmodel #194

Merged
merged 7 commits into from
Mar 21, 2023

Conversation

pbastide
Copy link
Member

@pbastide pbastide commented Feb 9, 2023

See issue #185.

I added a formula field to the PhyloNetworkLinearModel as suggested, and changed functions accordingly.

I also changed the field model to evomodel for the evolutionary model, to avoid any confusion between objects.

This is just a first attempt, I see at least a few TODOs:

  • Check the API : eg, the documentation refers to object.mm.m for the model matrix. We might need some dedicated functions to access what we really need, with functions such asformula, modelmatrix, modelframe, ...
  • Add deprecation notice
  • I added the formula after the fit using phylolm, changing the object after creation (see line 2450), I don't know if that is the best way to do it ?

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #194 (f4fe330) into master (948975d) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
+ Coverage   86.25%   86.32%   +0.06%     
==========================================
  Files          29       30       +1     
  Lines       12560    12562       +2     
==========================================
+ Hits        10834    10844      +10     
+ Misses       1726     1718       -8     
Impacted Files Coverage Δ
src/deprecated.jl 100.00% <100.00%> (ø)
src/traits.jl 97.23% <100.00%> (+0.28%) ⬆️
src/phyLiNCoptimization.jl 94.98% <0.00%> (+0.62%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cecileane
Copy link
Member

This is fantastic, thank you so much for getting the hard work done! phylolm becomes type-stable now that the main PhyloNetworkLinearModel is never wrapped up. It simplifies the code too, as there's no need to duplicate functions to apply to wrapped-up model. I like your change from model to evomodel for the name of the evolutionary model. Nice change for us to navigate the code. I also like the strategy you took (to update the formula field after the model is created).

  1. API: I agree that it would be nice of create function to access the model matrix and the formula, just like there's one to access the response. That should be easy to do.
  2. deprecation: I'm not sure we need this, because we are only changing the internal structure. The use of phylolm remains the same, and accessors to do inference also remain the same (coeftable etc.). But we should check them. For example, I get an error doing this --something that will go away after we take care of point 1.
julia> modelmatrix(fitBM)

ERROR: type PhyloNetworkLinearModel has no field pp

A bigger issue that I noticed after checking the documentation build is that the coefficient table doesn't have the good formula terms. Example from traits.jl around lines 521 from executing this:

julia> fitBM = phylolm(@formula(trait ~ shift_1 + shift_8), dfr, net; reml=false) # actual fit

I get this table:

Coefficients:
───────────────────────────────────────────────────────────────
       Coef.  Std. Error      t  Pr(>|t|)  Lower 95%  Upper 95%
───────────────────────────────────────────────────────────────
x1   9.48238    0.327089  28.99    0.0220    5.32632   13.6384
x2   3.9096     0.46862    8.34    0.0759   -2.04479    9.86399
x3  -2.4179     0.422825  -5.72    0.1102   -7.7904     2.95461
───────────────────────────────────────────────────────────────

instead of this expected table:

Coefficients:
────────────────────────────────────────────────────────────────────────
                Coef.  Std. Error      t  Pr(>|t|)  Lower 95%  Upper 95%
────────────────────────────────────────────────────────────────────────
(Intercept)   9.48238    0.327089  28.99    0.0220    5.32632   13.6384
shift_1       3.9096     0.46862    8.34    0.0759   -2.04479    9.86399
shift_8      -2.4179     0.422825  -5.72    0.1102   -7.7904     2.95461
────────────────────────────────────────────────────────────────────────

I will need to look into GLM more to see if they have a work around the issue.

@cecileane
Copy link
Member

Coefficient names are now correct in the coefficient table & summary, and methods are added for modelmatrix and formula on the fitted object.

@cecileane
Copy link
Member

useful link: how JuliaStats/GLM.jl#339 got rid of Tableregressionmodel.

@cecileane
Copy link
Member

I think it looks good. I only see some deprecation notice or some warning somewhere that we may try to add. But these changes will go v0.16.0 (increase from v0.15) anyway, because of breaking changes from a previous PR.

@pbastide : please "squash & merge" if you're happy with this.

@pbastide
Copy link
Member Author

Good catch for the coefnames !

I added deprecation notices as in JuliaStats/GLM.jl#339 as the previous documentation referred to object.mm.m, object.mf.f and object.model.

If tests still pass and you are ok with this, I think it's ready for a "squash and merge".

@cecileane cecileane merged commit 0083081 into JuliaPhylo:master Mar 21, 2023
@pbastide pbastide deleted the tableregressionmodel branch May 18, 2023 07:50
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.

2 participants