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.

Sign up for GitHub

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

Jump to bottom

the end of zerocorr! and checking consistent ReMat eltypes #406

Merged
merged 8 commits into from
Oct 7, 2020
Merged

the end of zerocorr! and checking consistent ReMat eltypes #406

merged 8 commits into from
Oct 7, 2020

Conversation

palday
Copy link
Member

@palday palday commented Oct 5, 2020

Closes #396.

@palday palday changed the title why zerocorr! can't be zerocorr why zerocorr! can't be zerocorr [ci skip] Oct 5, 2020
@codecov
Copy link

codecov bot commented Oct 5, 2020 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@3102f02). Click here to learn what that means.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #406   +/-   ##
=========================================
  Coverage          ?   94.30%           
=========================================
  Files             ?       23           
  Lines             ?     1632           
  Branches          ?        0           
=========================================
  Hits              ?     1539           
  Misses            ?       93           
  Partials          ?        0
Impacted Files Coverage Δ
src/MixedModels.jl 100.00% <ø> (ø)
src/remat.jl 95.37% <0.00%> (ø)
src/linearmixedmodel.jl 94.26% <100.00%> (ø)

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 3102f02...9cfebe4. Read the comment docs.

palday
palday commented Oct 5, 2020
View reviewed changes
src/linearmixedmodel.jl Outdated Show resolved Hide resolved
palday
palday commented Oct 5, 2020
View reviewed changes
src/linearmixedmodel.jl Outdated Show resolved Hide resolved
@palday palday requested a review from dmbates October 5, 2020 20:05
ararslan
ararslan reviewed Oct 5, 2020
View reviewed changes
src/linearmixedmodel.jl Outdated Show resolved Hide resolved
@palday @ararslan
Update src/linearmixedmodel.jl
b8fef0c 
Co-authored-by: Alex Arslan <ararslan@comcast.net>
dmbates
dmbates approved these changes Oct 6, 2020
View reviewed changes
Copy link
Collaborator

@dmbates dmbates left a comment

Choose a reason for hiding this comment

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

This discussion makes me think that removing zerocorr! altogether might be the best course.

@palday
Copy link
Member Author

palday commented Oct 6, 2020

I've been thinking that too. I wonder if we should remove it from the package but include it (or something like it) in the docs as an example of how advanced users can set restrictive variance structures.

@palday palday mentioned this pull request Oct 6, 2020
Open
@dmbates
Copy link
Collaborator

dmbates commented Oct 6, 2020

Sounds like a plan. Do you have time to do that or would you prefer that I do it?

@palday
Copy link
Member Author

palday commented Oct 6, 2020

I haven't added the docs yet, but I've marked zerocorr! (now _zerocorr!) as internal and updated the existing docs to not use zerocorr!. Along the way, I also found a few spots where #388 allows us to simplify the formulae and a spot where the type promotion caused an issue (because one of the ReMats was ReMat{Int8,S} and not promoted).

@palday
Copy link
Member Author

palday commented Oct 6, 2020 •
edited

I probably won't get around to a vignette on zerocorr! this week, so if you have time (and motivation), please feel free to do it. But missing vignettes aren't blockers for the big release, so also no rush.

@palday palday mentioned this pull request Oct 6, 2020
Closed
4 tasks
@palday palday changed the title why zerocorr! can't be zerocorr [ci skip] ~why zerocorr! can't be zerocorr~ the end of zerocorr!` and checking consistent ReMat eltypes Oct 6, 2020
@palday palday changed the title ~why zerocorr! can't be zerocorr~ the end of zerocorr!` and checking consistent ReMat eltypes the end of zerocorr! and checking consistent ReMat eltypes Oct 6, 2020
@palday
Copy link
Member Author

palday commented Oct 7, 2020

@dmbates Since I changed something in the LinearMixedModel constructor, I would prefer that you take one more look before I squash and merge.

dmbates
dmbates approved these changes Oct 7, 2020
View reviewed changes
Copy link
Collaborator

@dmbates dmbates left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for doing this.

docs/src/optimization.md Show resolved Hide resolved
src/linearmixedmodel.jl
@@ -78,6 +78,18 @@ function LinearMixedModel(
for (i, x) in enumerate(Xs)
if isa(x, AbstractReMat{T})
push!(reterms, x)
elseif isa(x, ReMat) # this can occur in weird situation where x is a ReMat{U}
# avoid keeping a second copy if unweighted
z = convert(Matrix{T}, x.z)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to write this construction as T.(x.z) but this is quite reasonable too.

@palday palday merged commit c23d43e into master Oct 7, 2020
@palday palday deleted the zerocorr! branch October 7, 2020 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@dmbates dmbates dmbates approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zerocorr! doesn't set all ReTerms to be ZeroCorr
3 participants
@palday @dmbates @ararslan