-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add extension for PDMats #303
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #303 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 7 8 +1
Lines 871 903 +32
=======================================
+ Hits 870 902 +32
Misses 1 1 ☔ View full report in Codecov by Sentry. |
I accept your argument for this being the right place for the extension to live. (This is a flaw in the extension setup that one package "owns" an extension so these arguments happen....) I personally don't particularly care about supporting <v1.9 but if someone else wants to comment on adding Requires.jl dependency please do. |
src/FillArrays.jl
Outdated
@static if !isdefined(Base, :get_extension) | ||
function __init__() | ||
Requires.@require PDMats = "90014a1f-27ba-587c-ab20-58faa44d9150" begin | ||
include("../ext/FillArraysPDMatsExt.jl") |
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 codecov annotation is wrong - the line is hit since otherwise tests on Julia 1.6 would fail.
Bump 🙂 |
ext/FillArraysPDMatsExt.jl
Outdated
@@ -0,0 +1,18 @@ | |||
module FillArraysPDMatsExt | |||
|
|||
if isdefined(Base, :get_extension) |
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 style seems a bit unusual to me. Would it not be possible to import the packages directly, or is the namespace specification necessary?
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.
The branch is necessary, Requires needs relative imports (as shown also in the docs: https://github.com/JuliaPackaging/Requires.jl and https://pkgdocs.julialang.org/v1/creating-packages/#Requires.jl).
Moreover, in an extension one should import only the package itself and its weak dependency, and even stdlibs should be loaded indirectly: JuliaStats/LogExpFunctions.jl#63
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.
Oh, interesting, I had no idea about this transitive loading issue. Looks fine to me then
Since |
I would prefer that actually but I wasn't sure if you would accept a dependency on PDMats when I opened the PR. What do you think @dlfivefifty? |
Gentle bump @dlfivefifty. I think it's better to avoid Requires unless absolutely necessary, and make it a hard dependency on old Julia versions. |
Why are you bumping me? This PR isn't ready, I agree it shouldn't add a dependency on Requires.jl |
I'm sorry, but I thought that this was stalled and waiting for your input |
We were waiting for an answer to #303 (comment). The PR was ready but apparently some recent changes on the master branch caused merge conflicts. I updated the PR and removed Requires: devmotion@e661a20 Edit: Seems there is some Github issue, the new commits don't show up in the PR (yet). |
Bump 🙂 |
This PR adds an extension for PDMats that is optionally loaded on Julia < 1.9 via Requires.
The sole purpose of the extension is to define the
PDMats.AbstractPDMat
constructor (added in JuliaStats/PDMats.jl#172) forDiagonal
matrices withAbstractFill
diagonals, so it is extremely lightweight. This constructor is very useful for packages that work with positive-definite matrices and use the type system and/or traits in PDMats and want to support generic user-provided matrices. A concrete use case is Distributions: For instance, with this extension the constructors ofMvNormal
in https://github.com/JuliaStats/Distributions.jl/blob/87aebc29b2b9608801b70aae09fbc1d2dad56e3f/src/multivariate/mvnormal.jl#L201-L210 (and similar constructors for other distributions that are constructed from positive-definite matrices) could be simplified toWithout this extension, a
Diagonal
matrix with anAbstractFill
diagonal would be converted to aPDiagMat
, a type for positive-definite diagonal matrices, instead of the more efficientScalMat
type for positive-definite scaled identity matrices.Originally, I proposed to add this extension to PDMats (JuliaStats/PDMats.jl#192) but after some discussions it was decided that it would be more suitable to add the extension to FillArrays: As pointed out in JuliaStats/PDMats.jl#182 (comment), currently PDMats loads very quickly compared with FillArrays (possibly due to invalidations?). For instance on my computer the latest releases of FillArrays and PDMats load in
Conceptually, it is also not feasible to add extensions for every custom array type to PDMats, so adding an extension to FillArrays and, whenever suitable, to other array packages seems more practical.
It also seems that this PR does not have a significant impact on loading times of FillArrays. With this PR, on Julia 1.9.3 I see