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

Crout Decomposition #52

Merged
merged 16 commits into from Jul 2, 2013
Merged

Crout Decomposition #52

merged 16 commits into from Jul 2, 2013

Conversation

@yuronew
Copy link
Contributor

yuronew commented Jun 29, 2013

my first pull request)

@vkostyukov
Copy link
Owner

vkostyukov commented Jun 30, 2013

Hi Yuriy @yuronew,

Thank you for the contribution! I have few questions:

  1. Is Crout method faster then triangularize().product(), which is used currently?
  2. Is your implementation faster then call to existed LU decompositor: decompose(Matrices.LU_DECOMPOSITOR)[0].product()?

So, if yes, we need replace current implementation of determinant method with your Crout or default LU. The only line should be replaced is triangularize().product().

Please, try to collect some performance data for this.

yuronew added 2 commits Jun 30, 2013
changed determinant searching, now CroutDecompositor is used
added Crout_DECOMPOSITOR instance to Matrices class
added test case for CroutDecompositor
@vkostyukov
Copy link
Owner

vkostyukov commented Jun 30, 2013

@yuronew, could you please remove "bin" and other files from pull-request? You can do it by changing the last commit git commit --amend.

/**
* The {@link CroutDecompositor} singleton instance.
*/
public final static MatrixDecompositor Crout_DECOMPOSITOR =

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

It should be CROUT_DECOMPOSITOR.

*
* @return the "determinant" of this matrix
*/
double crout();

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

We don't need having this method in Matrix interface. It is enough to have determinant().


Matrix l = factory.createMatrix(matrix.rows(), matrix.columns());
Matrix u = factory.createIdentityMatrix(matrix.rows());
double s=0;

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

Please, declare variable just before using.

double s=0;
for (int j = 0; j < l.columns(); j++){
for (int i = j; i < l.rows(); i++){
s=0;

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

Here double s = 0.0;

}

for (int i = j; i < l.rows(); i++){
s=0;

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

Declare a new double variable here.

for (int k = 0; k < j; k++){
s = s + l.get(j, k)*u.get(k, i);
}
if (l.get(j, j) == 0) throw new IllegalArgumentException("Singular matrix!");

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

We can't compare double with 0.0 - it is unsafe. Please, use the following code:

if (Math.abs(value) < Matrices.EPS) {
  // close to zero
}

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

Also, add brackets after condition.

@@ -953,4 +954,38 @@ protected void ensureDimensionsAreNotNegative(int rows, int columns) {
+ rows + "x" + columns);
}
}

@Override
public double crout(){

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

We don't need this method.

@vkostyukov
Copy link
Owner

vkostyukov commented Jun 30, 2013

@yuronew,

I've came through the patch. Please, make sure that you use spaces instead of tabs. Configure your IDE and try to figure out why it doesn't look like the rest of la4j's code.

Also, please follow the code convetion (by Oracle). There is no much spaces in you code. In statemens like:

a.set(i,j,5+4) -> a.set(i, j, 5 + 4);

Please, also follow my comments to improve your patch.

yuronew added 6 commits Jun 30, 2013
changed determinant searching, now CroutDecompositor is used
added Crout_DECOMPOSITOR instance to Matrices class
added test case for CroutDecompositor
@@ -511,4 +511,9 @@ protected void ensureIndexInColumns(int i) {
+ i);
}
}

This comment has been minimized.

@vkostyukov

vkostyukov Jun 30, 2013 Owner

There is a compile error. This method no longer exsits.

@vkostyukov
Copy link
Owner

vkostyukov commented Jun 30, 2013

Hi @yuronew,

Just one thing. Please remove crout method. It raises compile error.

@vkostyukov
Copy link
Owner

vkostyukov commented Jul 1, 2013

Hi @yuronew,

There is still a failed test:

Running org.la4j.decomposition.CroutDecompositorTest
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.012 sec <<< FAILURE!

Please, run mvn test to make sure all tests are passed.

root and others added 5 commits Jul 1, 2013
@vkostyukov
Copy link
Owner

vkostyukov commented Jul 2, 2013

Thanks Yuriy!

vkostyukov added a commit that referenced this pull request Jul 2, 2013
Crout Decomposition
@vkostyukov vkostyukov merged commit 135ca3c into vkostyukov:master Jul 2, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.