Skip to content

Add more functions based on Cholesky decomposition #79

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

Merged
merged 4 commits into from
Sep 27, 2017

Conversation

jturner314
Copy link
Member

This PR has a few components:

  • Rename Absolute::squared() to Absolute::abs_sqr(). (See the commit message for the reasoning.) Note that this is a breaking change.
  • Make the various .cholesky*() methods return a FactorizedCholesky struct instead of the L or U matrix directly. Note that this is a breaking change.
  • Provide methods on the FactorizedCholesky struct for getting the other decomposition (L instead of U or vice versa), calculating the inverse, calculating the determinant (or natural log of the determinant), and solving linear equations.

Please let me know what you think. I wasn't sure if you'd like for the inverse and solving methods to be made into separate traits with unique names, like in the solveh module. Personally, I prefer this implementation, where instead everything is a method on the FactorizedCholesky type. This way, not every method has to have to have a unique name (e.g. .solve() and .into_inverse()).

For complex numbers, the result of this function is actually the
square of the absolute value of the number, not the square of the
number.

|a + bi|^2 == a^2 + b^2 == (a + bi)(a - bi) != (a + bi)^2
The body of `cholesky_t()` was identical to the body of `cholesky()`.
This provides additional operations based on Cholesky decomposition,
including matrix inverses, determinants, and solutions to linear
systems.
@termoshtt
Copy link
Member

Rename Absolute::squared() to Absolute::abs_sqr(). (See the commit message for the reasoning.) Note that this is a breaking change.

I agree that squared is misleading, and think the new name abs_sqr is good.

I wasn't sure if you'd like for the inverse and solving methods to be made into separate traits with unique names, like in the solveh module.

Hum... the unique name (e.g. inv() and invh()) is useful for functions for ArrayBase since we cannot switch them automatically:

let a = random((n, n));
a.inv();  // use LU factorization
a.invh(); // use BK pivoting
a.invc(); // use Cholesky factorization (to be implemented?)

Hence I need InverseH* trait at least for ArrayBase.
On the other hand, I have no strong reason to use SolveH trait for FactorizedH, they can be associated function of FactorizedH and use like solve and into_inverse naming like you done.

My request is following:

  • use factorizec() instead of cholesky (also for _into and _mut)
  • Add function cholesky which do factorizec() and into_lower() to recover current cholesky function
  • Rename Factorize(d)Cholesky into CholeskyFactorize(d)
    • I will rename Factorize(d) into LUFactorize(d) and Factorize(d)H into BKFactorize(d)

@termoshtt termoshtt added the breaking change Non-compatible change label Sep 22, 2017
@termoshtt
Copy link
Member

Rather, I will replace the associated function into_inverse by the trait InverseInto on #81. Then the following codes become identical:

let a = random((n, n));
let a_inv = a.inv_into();
let a = random((n, n));
let f = a.factorize();
let a_inv = f.inv_into();

Then the role of LUFactorized is to avoid executing the factorization twice. a and f has same interaface and f exists only for the speed.

Please define the functions det() and into_lower() as trait, and implement them to both CholeskyFactorized and ArrayBase.

@jturner314
Copy link
Member Author

I just pushed a new version that I think does everything you asked for, with one exception: implementing into_lower() for ArrayBase. The purpose of the into_lower() and into_upper() methods on CholeskyFactorized is to make it easy to get the L or U matrix from the factorized struct. I suppose you could add a method to ArrayBase named into_lowerc() or something (which would be equivalent to cholesky_into(UPLO::Lower)) , but I don't think it makes sense to implement into_lower() for ArrayBase because it's unclear from its name what into_lower() would mean for a general matrix.

@termoshtt
Copy link
Member

I don't think it makes sense to implement into_lower() for ArrayBase because it's unclear from its name what into_lower() would mean for a general matrix.

You are correct. It should be an associated function.

@termoshtt termoshtt merged commit d5898c4 into rust-ndarray:master Sep 27, 2017
@jturner314 jturner314 deleted the add-cholesky-fac branch September 27, 2017 03:08
@termoshtt termoshtt added this to the v0.7.0 milestone Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Non-compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants