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

inv(::StaticMatrix) should always return StaticMatrix #34

Closed
wants to merge 1 commit into from
Closed

inv(::StaticMatrix) should always return StaticMatrix #34

wants to merge 1 commit into from

Conversation

nbaum
Copy link
Contributor

@nbaum nbaum commented Sep 25, 2016

Currently, inv(SMatrix{4, 4}(rand(Float32, 4, 4))) returns an Array{Float32, 2}.

I think it should return a SMatrix{4, 4}.

Currently, `inv(SMatrix{4, 4}(rand(Float32, 4, 4)))` returns an `Array{T, Float32}`.

I think it should return a `SMatrix{4, 4}`.
@andyferris
Copy link
Member

Currently only up to 3x3 returns SMatrix with specialised (fast) algorithms. So far things that aren't specialised return Matrix or MMatrix and this is one way users can explore what has been implemented faster than in Base.

Not that this PR is a bad idea at all, just I was hoping to either have a specialised algorithm (maybe just 4x4) or a way of avoiding the extra allocations within Julia to make it a bit speedier.

@andyferris
Copy link
Member

By the way, this method was copied from Base.

@andyferris
Copy link
Member

Other opinions? @c42f @SimonDanisch

@c42f
Copy link
Member

c42f commented Sep 29, 2016

FixedSizeArrays has inverses up to 4x4; looks like they're based on cofactors at a quick glance. I don't know off the top of my head (and couldn't immediately find) a definitive numerical method for small matrices.

As for this PR, I think it's good to make the output type SMatrix because it makes the mutablility of the result more consistent. 👍 from me.

@andyferris
Copy link
Member

andyferris commented Oct 3, 2016

Let's see if we can get JuliaLang/LinearAlgebra.jl#370 fixed quickly, or else write our own workarounds. Then lufact would work properly and inv would fall back gracefully to MMatrix like eig does (rather than Array like inv does now).

@c42f Do you think preserving immutability is very important? Preserving the static size, yes, but from a performance perspective the problem is largely the allocation and GC which is the same either way (an allocation is done in the working code in Base). With an MMatrix you may avoid unnecessary copies to and around the stack...

@c42f
Copy link
Member

c42f commented Oct 3, 2016

Hmm, well type inference should be fine either way. Performance-wise, forcing a copy into an SMatrix will presumably be slightly slower as you say.

To me it's just more consistent at the API level if the output type is always an SMatrix. Currently:

m = blah()
invm = inv(m)
invm[1,1] = 1 # succeeds or fails, depending on implementation details of inv().

I dunno, the size dependent behaviour just seems a bit unfortunate.

@andyferris
Copy link
Member

I dunno, the size dependent behaviour just seems a bit unfortunate.

Very true.

@andyferris
Copy link
Member

At the moment this returns a SizedArray... is this sufficient @nbaum?

@c42f
Copy link
Member

c42f commented Aug 1, 2019

I think the underlying issue here has been solved for some time.

@c42f c42f closed this Aug 1, 2019
@c42f c42f added bugfix design speculative design related issue labels Aug 1, 2019
oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix design speculative design related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants