Remove inv_value and inv_diag
#146
Conversation
| @@ -132,7 +127,7 @@ end | |||
| ### tri products | |||
|
|
|||
| function X_A_Xt(a::PDiagMat, x::StridedMatrix) | |||
| z = x .* reshape(sqrt.(a.diag), 1, dim(a)) | |||
| z = x .* sqrt.(reshape(a.diag, 1, dim(a))) | |||
There was a problem hiding this comment.
This (and similar line below) should be fixed in any case. It seems the current implementation prevents broadcast fusion and creates an additional intermediate array.
src/scalmat.jl
Outdated
| @@ -92,10 +89,10 @@ end | |||
|
|
|||
| function X_invA_Xt(a::ScalMat, x::StridedMatrix) | |||
| @check_argdims dim(a) == size(x, 2) | |||
| lmul!(a.inv_value, x * transpose(x)) | |||
| ldiv!(a.value, x * transpose(x)) | |||
There was a problem hiding this comment.
It seems ldiv!(::Number, ::AbstractArray) is not available on Julia 1.0. I did not realize this since there is no note in the docstring: https://docs.julialang.org/en/v1/stdlib/LinearAlgebra/#LinearAlgebra.ldiv!
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
==========================================
- Coverage 88.85% 88.33% -0.53%
==========================================
Files 8 8
Lines 377 403 +26
==========================================
+ Hits 335 356 +21
- Misses 42 47 +5
Continue to review full report at Codecov.
|
|
@Wynand I also added your failing example as a test. |
|
Should we consider this a breaking change? |
1 similar comment
|
Should we consider this a breaking change? |
|
It is supposed to be non-breaking - I deprecated the constructors but considered the fields to be not part of the public API (alternatively, I could add a deprecation warning in |
|
It would at least break https://github.com/devmotion/CalibrationErrorsDistributions.jl/blob/41be6f90f9cf57e3f9e8333d3d1f4eb73d55002e/src/mvnormal.jl#L106 which might be familiar to you so maybe let's bump the minor version. |
|
I went through all the direct dependencies and yours is the only one that touches the field so I'd also be okay with just making it a patch release. |
|
Ah I'd like to avoid breaking my package, even if it accesses (internal?) fields 😄 I'll update the package in a way that does not use |
|
I fixed CalibrationErrorsDistributions and updated the README which I forgot initially. |
Now I'm fine with it as well 🙂 |
| @@ -1,6 +1,6 @@ | |||
| name = "PDMats" | |||
| uuid = "90014a1f-27ba-587c-ab20-58faa44d9150" | |||
| version = "0.11.3" | |||
| version = "0.11.4" | |||
There was a problem hiding this comment.
Should this have bumped to 0.11.5? It looks like 0.11.4 was already released when this was merged: https://github.com/JuliaStats/PDMats.jl/releases/tag/v0.11.4
There was a problem hiding this comment.
Yes, it should, JuliaRegistrator noticed it as well: 2f3a83e#commitcomment-60431201 The PR was created before the other PR was merged and released, so the diff in the PR was outdated and based on an earlier version of the master branch.
Alternative to #141 that removes
inv_valueandinv_diagfields fromScalMatandPDiagMat.Advantages compared with the current master branch are that there are no problems (incompatible types or undesired type promotions) with vectors and values where the inverse are of different type. Two helper functions are added that use division instead of operating with
inv(value)directly to avoid undesired type promotions (e.g., ifx isa AbstractVector{Float32}andvalue isa Int, theninv(value) * x isa AbstractVector{Float64}whereasx / value isa AbstractVector{Float32}).