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

Allow for offset in GLMMs and simplify wts #482

Merged
merged 7 commits into from
Mar 17, 2021
Merged

Allow for offset in GLMMs and simplify wts #482

merged 7 commits into from
Mar 17, 2021

Conversation

dmbates
Copy link
Collaborator

@dmbates dmbates commented Mar 4, 2021

@dmbates dmbates marked this pull request as draft March 4, 2021 23:33
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #482 (d682d28) into master (135e87d) will increase coverage by 0.65%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
+ Coverage   93.36%   94.01%   +0.65%     
==========================================
  Files          27       27              
  Lines        1929     2140     +211     
==========================================
+ Hits         1801     2012     +211     
  Misses        128      128              
Impacted Files Coverage Δ
src/generalizedlinearmixedmodel.jl 89.16% <100.00%> (+0.99%) ⬆️
src/linearmixedmodel.jl 95.44% <100.00%> (+0.43%) ⬆️
src/likelihoodratiotest.jl 97.64% <0.00%> (-1.04%) ⬇️
src/blocks.jl 100.00% <0.00%> (ø)
src/varcorr.jl 100.00% <0.00%> (ø)
src/datasets.jl 100.00% <0.00%> (ø)
src/utilities.jl 100.00% <0.00%> (ø)
src/optsummary.jl 100.00% <0.00%> (ø)
src/gausshermite.jl 100.00% <0.00%> (ø)
src/linalg/pivot.jl 100.00% <0.00%> (ø)
... and 17 more

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 135e87d...2474373. Read the comment docs.

@dmbates dmbates requested a review from palday March 16, 2021 17:33
@dmbates
Copy link
Collaborator Author

dmbates commented Mar 16, 2021

@palday Should we go ahead and merge this, perhaps after turning off the Future ci. I would say that this is a bug fix. (I suppose now you want me to put it in News.md, don't you?)

@palday
Copy link
Member

palday commented Mar 16, 2021

@dmbates you predicted my request. 😄 Can you put it under 3.4.1 since it's technically a bug fix?

As a sidebar: should we support offsets in LMM (in a separate PR)?

@palday palday marked this pull request as ready for review March 16, 2021 18:25
@dmbates
Copy link
Collaborator Author

dmbates commented Mar 17, 2021

This branch is currently based on the master branch because I was intending it to check off one of the milestones for v4.0.0, I know it is a bug fix and could be incorporated as v3.4.1 but I am not sure my git skills are up to backing out the other changes in the master branch.

@palday
Copy link
Member

palday commented Mar 17, 2021

Put the NEWS entry in the right spot, we'll merge into master and I'll git-magic the backport aftwards. 😄

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 17, 2021

As a sidebar: should we support offsets in LMM (in a separate PR)?

The uses of an offset for a model with identity link have always seemed rather contrived to me. You might as well just shift the response by the offset and proceed normally.

@palday
Copy link
Member

palday commented Mar 17, 2021

The uses of an offset for a model with identity link have always seemed rather contrived to me. You might as well just shift the response by the offset and proceed normally.

My thoughts exactly. 😄 Should we error if an offset is specified for an LMM then?

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 17, 2021

The uses of an offset for a model with identity link have always seemed rather contrived to me. You might as well just shift the response by the offset and proceed normally.

My thoughts exactly. smile Should we error if an offset is specified for an LMM then?

Yes, please do.

@palday
Copy link
Member

palday commented Mar 17, 2021

The constructor clearly will error, but for the fit(MixedModel, formula, data; offset=data.o) I don't know what will happen.

I'll add it later today and then get this merged in. 😄

@palday palday merged commit 9fc9e50 into master Mar 17, 2021
@palday palday deleted the glmmoffset branch March 17, 2021 16:58
palday pushed a commit that referenced this pull request Mar 17, 2021
* Allow for offset in GLMMs and simplify wts

* Add dataset and test for offset in Poisson GLMM

* Add a NEWS item.

* error on LMM offset

* news formatting

* NEWS links

Co-authored-by: Phillip Alday <[email protected]>
(cherry picked from commit 9fc9e50)
palday added a commit that referenced this pull request Mar 17, 2021
* bring back LazyArtifacts (#486)

* bring back LazyArtifacts

* drop Pkg dep

* compat entry for lazyartifacts (not stdlib on julia <1.6)

(cherry picked from commit 135e87d)

* Allow for offset in GLMMs and simplify wts (#482)

* Allow for offset in GLMMs and simplify wts

* Add dataset and test for offset in Poisson GLMM

* Add a NEWS item.

* error on LMM offset

* news formatting

* NEWS links

Co-authored-by: Phillip Alday <[email protected]>
(cherry picked from commit 9fc9e50)

* patch bump

Co-authored-by: Douglas Bates <[email protected]>
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.

Offset for Poisson models
2 participants