-
Notifications
You must be signed in to change notification settings - Fork 48
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
Small performance tweaks #772
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #772 +/- ##
=======================================
Coverage 96.93% 96.93%
=======================================
Files 34 34
Lines 3356 3358 +2
=======================================
+ Hits 3253 3255 +2
Misses 103 103
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -408,7 +408,7 @@ function createAL(reterms::Vector{<:AbstractReMat{T}}, Xy::FeMat{T}) where {T} | |||
end | |||
end | |||
end | |||
return A, L | |||
return identity.(A), identity.(L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will tighten up the eltype of the vector when all elements are of a single type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a good idea but I don't think it will change things in the current setup. At least for L
, first(L)
is always Diagonal
or UniformBlockDiagonal
and last{L)
is always Matrix{T}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that matches my own sanity checks. I have some further ideas building upon this, but haven't had time to experiment more. 😄
b4d32e1
to
1bab6e7
Compare
@@ -31,63 +31,40 @@ The latter four fields are MixedModels functionality and not related directly to | |||
the future to use a different subtype of `AbstractVector` (e.g., `StaticArrays.SVector`) | |||
for each snapshot without being considered a breaking change. | |||
""" | |||
mutable struct OptSummary{T<:AbstractFloat} | |||
Base.@kwdef mutable struct OptSummary{T<:AbstractFloat} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that this will make some things easier to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
….jl into pa/type_inference
MixedModels v4.24.1 Release Notes | ||
============================== | ||
Add type notations in `pwrss(::LinearMixedModel)` and `logdet(::LinearMixedModel)` to enhance type inference. [#773] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking up that I had set the version to the wrong value and had the notes in the wrong place.
A::Vector{AbstractMatrix{T}} # cross-product blocks | ||
L::Vector{AbstractMatrix{T}} | ||
A::Vector{<:AbstractMatrix{T}} # cross-product blocks | ||
L::Vector{<:AbstractMatrix{T}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, thanks for picking this up. This is a sign that you write a lot of Julia code.
@@ -31,63 +31,40 @@ The latter four fields are MixedModels functionality and not related directly to | |||
the future to use a different subtype of `AbstractVector` (e.g., `StaticArrays.SVector`) | |||
for each snapshot without being considered a breaking change. | |||
""" | |||
mutable struct OptSummary{T<:AbstractFloat} | |||
Base.@kwdef mutable struct OptSummary{T<:AbstractFloat} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
Thanks for contributing!
Did behavior change? Did you add need features? If so, please update NEWS.md
docs/NEWS-update.jl
to update the cross-references.Should we release your changes right away? If so, bump the version: