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

More conversion in PDMat #212

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

timholy
Copy link
Contributor

@timholy timholy commented Dec 10, 2024

This allows one to convert both the eltype and matrix type. In practice, my main use for this is to support generic programming with both Matrix and StaticArrays.SMatrix.

Perhaps the only potentially-controversial aspect is moving the dimensionality check into the inner constructor. If we want to make it impossible to construct an inconsistent PDMat, that's where it has to be.

@devmotion
Copy link
Member

Did you check whether this is covered by #188? At least it seems the matrix element type is restricted in the same way in #188.

@timholy
Copy link
Contributor Author

timholy commented Dec 10, 2024

Did you check whether this is covered by #188? At least it seems the matrix element type is restricted in the same way in #188.

The eltype restriction is just a small cleanup. The key aspect of this is the "constructor cascade" that supports conversion/coercion via PDMat{T}(args...) or PDMat{T,S}(args...). This is needed to support, e.g., push!(pdmats, pdmat) where eltype(pdmats) may not be identical to typeof(pdmat). #188 doesn't add these methods.

Also, #188 is a pretty deep change, and it's been open for more than a year. I hoped this could get merged quickly and a new release made.

@timholy
Copy link
Contributor Author

timholy commented Dec 10, 2024

Concretely, if one runs the test block "test the functionality" from this PR on master, one gets this (with T = Float32):

Testing PDMat{Float32, Matrix{Float32}} of size (3, 3)
PDMat from Matrix: Error During Test at REPL[8]:7
  Got exception outside of a @test
  MethodError: no method matching (PDMat{Float64})(::Matrix{Float32})
  Stacktrace:

and several similar errors.

src/pdmat.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Contributor Author

timholy commented Dec 10, 2024

If you're OK with it, I'd plan to solve the failure on Julia 1.0 by dropping support for anything before 1.10. Since this package is still in 0.x releases, we don't have to bump the minor version to do so.

The CI error on nightly is unrelated to this PR.

src/pdmat.jl Outdated Show resolved Hide resolved
src/pdmat.jl Outdated Show resolved Hide resolved
src/pdmat.jl Outdated Show resolved Hide resolved
src/pdmat.jl Outdated Show resolved Hide resolved
src/pdmat.jl Outdated Show resolved Hide resolved
src/pdmat.jl Outdated Show resolved Hide resolved
src/chol.jl Outdated Show resolved Hide resolved
timholy and others added 6 commits December 10, 2024 09:10
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
src/pdmat.jl Outdated Show resolved Hide resolved
src/pdmat.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

If you're OK with it, I'd plan to solve the failure on Julia 1.0 by dropping support for anything before 1.10.

I'm fine with that but I'm still curious which and why some of the changes are problematic on Julia 1.0.

@timholy
Copy link
Contributor Author

timholy commented Dec 11, 2024

I'm fine with that but I'm still curious which and why some of the changes are problematic on Julia 1.0.

I'm a little puzzled about that as well. But it's only the @allocated test that fails and that is indeed pretty sensitive (especially in old Julias) to minor details. Looking at the produced code

julia> @code_typed PDMat(S, C)
CodeInfo(
22 1%1 = %new(PDMat{Float64,SArray{Tuple{4,4},Float64,2,16}}, mat, chol)::PDMat{Float64,SArray{Tuple{4,4},Float64,2,16}}                       │╻ Type
   └──      return %1                                                                                                                             │
) => PDMat{Float64,SArray{Tuple{4,4},Float64,2,16}}

it's a little weird that it allocates at all.

I dunno, I am not personally that interested in pursuing why Julia 1.0 allocates in this circumstance.

I was hoping this solves the allocations on 1.0
@timholy
Copy link
Contributor Author

timholy commented Dec 11, 2024

You may want to think about whether we want a5c0ce8, I added it hoping that it would fix the allocations on Julia 1.0. But it doesn't, and it makes the code more verbose; on the other hand, it tightens the applicability of the methods to things that can actually work. Let me know whether you'd prefer to drop it or keep it.

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