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

Xy block #464

Merged
merged 29 commits into from
Mar 2, 2021
Merged

Xy block #464

merged 29 commits into from
Mar 2, 2021

Conversation

dmbates
Copy link
Collaborator

@dmbates dmbates commented Feb 13, 2021

  • Replace the last two blocks, X and y, in the terms field with a single femat matrix that includes both X and y
  • This cuts down on the number of calls to multiplication, etc. in updateL! and allows multi-threaded BLAS to be more effective
  • This PR is incomplete

@dmbates dmbates marked this pull request as draft February 13, 2021 00:38
@dmbates dmbates requested a review from palday February 13, 2021 00:39
src/linearmixedmodel.jl Outdated Show resolved Hide resolved
@dmbates
Copy link
Collaborator Author

dmbates commented Feb 13, 2021

  • Changes in the GLMM code to accommodate the new structures.
  • Several changes are because the GLM package specializes some methods to Vector{T}. If those were changed to AbstractVector{T} the changes would not be necessary.
  • Some of the changes of arguments like foo.bar.baz' to adjoint(foo.bar.baz) were simply to get the linter to shut up. They were being flagged as syntax errors despite the fact that they were not syntax errors.

@palday
Copy link
Member

palday commented Feb 14, 2021

Thanks for the explanatory comments. :) Should we open a PR against GLM for changing loosening the type requirements?

test/pls.jl Show resolved Hide resolved
@dmbates
Copy link
Collaborator Author

dmbates commented Feb 25, 2021

@palday I am done tinkering. Now if you could just do the heavy lifting of resolving conflicts, etc.

src/linalg.jl Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #464 (ba2961f) into master (f81ccbc) will decrease coverage by 0.06%.
The diff coverage is 95.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
- Coverage   93.49%   93.43%   -0.07%     
==========================================
  Files          27       27              
  Lines        1906     1949      +43     
==========================================
+ Hits         1782     1821      +39     
- Misses        124      128       +4     
Impacted Files Coverage Δ
src/MixedModels.jl 100.00% <ø> (ø)
src/arraytypes.jl 93.47% <0.00%> (+2.17%) ⬆️
src/blockdescription.jl 100.00% <ø> (ø)
src/generalizedlinearmixedmodel.jl 88.16% <84.61%> (-0.07%) ⬇️
src/Xymat.jl 86.84% <86.84%> (ø)
src/linalg.jl 59.72% <100.00%> (ø)
src/linalg/logdet.jl 100.00% <100.00%> (ø)
src/linearmixedmodel.jl 95.01% <100.00%> (+0.70%) ⬆️
src/mixedmodel.jl 83.33% <100.00%> (ø)
src/remat.jl 95.35% <100.00%> (-0.02%) ⬇️
... and 5 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 f81ccbc...63c32db. Read the comment docs.

src/linalg/logdet.jl Show resolved Hide resolved
src/Xymat.jl Show resolved Hide resolved
src/Xymat.jl Show resolved Hide resolved
src/Xymat.jl Show resolved Hide resolved
src/Xymat.jl Outdated Show resolved Hide resolved
src/linearmixedmodel.jl Show resolved Hide resolved
test/matrixterm.jl Outdated Show resolved Hide resolved
test/matrixterm.jl Outdated Show resolved Hide resolved
test/matrixterm.jl Outdated Show resolved Hide resolved
test/pls.jl Outdated Show resolved Hide resolved
@palday
Copy link
Member

palday commented Feb 26, 2021

The failure on Future is something I've fixed as part of my changes in #474

@dmbates
Copy link
Collaborator Author

dmbates commented Feb 26, 2021

In v"1.7.0-DEV.620" and later, which use libblastrampoline, the first use of BLAS.vendor() produces a deprecation warning. I can commit a change for that if it seems worthwhile.

@palday
Copy link
Member

palday commented Feb 26, 2021

Go for it -- we've already got a @static guard there noting that I would still like to have tests notify us of platform specifics (since those have been the source of problems in the past).

@dmbates
Copy link
Collaborator Author

dmbates commented Feb 26, 2021

I think we should look at pivotedQR versus pivotedCholesky for determining the rank of X after we merge this. So probably don't make a new release immediately.

@palday
Copy link
Member

palday commented Feb 26, 2021

I also think there's a few things to think about before we put a bow on 4.0.

@palday
Copy link
Member

palday commented Feb 26, 2021

There's also something I'm currently tracking down with the new show methods. In my local build of the docs, the markdown is used and renders correctly (not what we want for showing users how to step through the REPL, but also at least understandable). But the GHA builds all have the markdown output but not rendered into HTML. I'm working on figuring that out.

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 1, 2021

@palday Let me know if I should be looking at this PR or if you are ready to do the heavy git lifting to make it compatible with current master.

@palday
Copy link
Member

palday commented Mar 1, 2021

I already did the lifting. 😄

@dmbates
Copy link
Collaborator Author

dmbates commented Mar 1, 2021

I think this is ready to go. If you agree @palday please go ahead and merge.

@palday palday marked this pull request as ready for review March 2, 2021 12:24
@palday palday merged commit 26a84fc into master Mar 2, 2021
@palday palday deleted the XyBlock branch March 2, 2021 13:24
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