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

Determinants #53

Merged
merged 2 commits into from Feb 10, 2014

Conversation

Projects
None yet
3 participants
@jeffreyrosenbluth
Member

jeffreyrosenbluth commented Feb 10, 2014

I think this is what was intended by the trello card "implement determinants".

Not the most efficient algorithm, but most of our transformations are of low dimension and the cost of converting to a more efficient data structure seems to out weigh the benefits, fro example the following implementation is about 4X slower on 4 x 4 matrices when taking into account the time needed to convert to Data.Vectors. Additionally, something like an LU decomposition seems like overkill.

minorV ::  Int -> Int -> Vector (Vector a) -> Vector (Vector a)
minorV i j v = removeV j $ V.map (removeV i) v
  where removeV n = V.ifilter (\k _ -> n /= k)

detV :: Num a => Vector (Vector a) -> a
detV m
  | V.length m == 1 = V.head . V.head $ m
  | otherwise = sum [(-1)^i * (c1 ! i) * detV (minorV i 0 m) | i <- [0 .. (n-1)]]
      where
        c1 = V.head m
        n = V.length m

It is also possible that matrixRep or something like it can be used instead of the definitions of onBasis in the individual vector spaces, e.g. R2, R3.

@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Feb 10, 2014

Member

Looks good to me. I'm surprised by the vector result; thanks for profiling that. Do you want to export determinant from Diagrams.Core? I think it's mostly useful inside Backends, and it's easier to find if exported there.

I'd like to give other folks more time to review, but I'm OK merging this tomorrow if they're busy.

onBasis looks the way it does because I don't like exporting lists with a promise that they'll always be a known length. I like having the type checker verify that. But I don't feel that strongly about it.

Member

bergey commented Feb 10, 2014

Looks good to me. I'm surprised by the vector result; thanks for profiling that. Do you want to export determinant from Diagrams.Core? I think it's mostly useful inside Backends, and it's easier to find if exported there.

I'd like to give other folks more time to review, but I'm OK merging this tomorrow if they're busy.

onBasis looks the way it does because I don't like exporting lists with a promise that they'll always be a known length. I like having the type checker verify that. But I don't feel that strongly about it.

@byorgey

This comment has been minimized.

Show comment
Hide comment
@byorgey

byorgey Feb 10, 2014

Member

The code looks good to me; has anyone tested it to make sure it produces correct results?

Member

byorgey commented Feb 10, 2014

The code looks good to me; has anyone tested it to make sure it produces correct results?

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Feb 10, 2014

Member

I didn't export determinant from Diagrams.Core because I was following the lead of onBasis but I am happy to do so.
I tested the code on Transformations of type T2 and T3 and det on some 4 x 4s. All giving correct results.

Member

jeffreyrosenbluth commented Feb 10, 2014

I didn't export determinant from Diagrams.Core because I was following the lead of onBasis but I am happy to do so.
I tested the code on Transformations of type T2 and T3 and det on some 4 x 4s. All giving correct results.

@bergey

This comment has been minimized.

Show comment
Hide comment
@bergey

bergey Feb 10, 2014

Member

I didn't export onBasis because I wanted to steer Backend writers to the better-typed versions in TwoD.Transform and ThreeD.Transform.

Member

bergey commented Feb 10, 2014

I didn't export onBasis because I wanted to steer Backend writers to the better-typed versions in TwoD.Transform and ThreeD.Transform.

@jeffreyrosenbluth

This comment has been minimized.

Show comment
Hide comment
@jeffreyrosenbluth

jeffreyrosenbluth Feb 10, 2014

Member

Got it, I'll export determinate then.

Member

jeffreyrosenbluth commented Feb 10, 2014

Got it, I'll export determinate then.

bergey added a commit that referenced this pull request Feb 10, 2014

Merge pull request #53 from diagrams/det
Determinants of Transformations

@bergey bergey merged commit cbf4d97 into master Feb 10, 2014

1 check passed

default The Travis CI build passed
Details

@jeffreyrosenbluth jeffreyrosenbluth deleted the det branch Feb 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment