Hipparchus-Math / hipparchus

An efficient, general-purpose mathematics components library in the Java programming language
Apache License 2.0
142 stars 41 forks source link

SNAPSHOT 2.0 - sum.getReal() for symbolic matrices? #134

Closed axkr closed 3 years ago

axkr commented 3 years ago

This call now creates an exception:

Can we have an option to skip this new feature or a dedicated method which returns the largest element.

My test cases for symbolic matrices fail now because of the getReal() method:

org.matheclipse.core.eval.exception.ArgumentTypeException: conversion into a machine-size double numeric value is not possible!
    at org.matheclipse.core.eval.EvalEngine.evalDouble(EvalEngine.java:1183)
    at org.matheclipse.core.interfaces.IExpr.evalDouble(IExpr.java:645)
    at org.matheclipse.core.interfaces.IExpr.getReal(IExpr.java:655)
    at org.hipparchus.linear.FieldLUDecomposition.<init>(FieldLUDecomposition.java:144)
    at org.hipparchus.linear.FieldLUDecomposition.<init>(FieldLUDecomposition.java:89)
    at org.matheclipse.core.builtin.LinearAlgebra$Inverse.inverseMatrix(LinearAlgebra.java:2142)
    at org.matheclipse.core.builtin.LinearAlgebra$Inverse.matrixEval(LinearAlgebra.java:2155)
    at org.matheclipse.core.eval.interfaces.AbstractMatrix1Matrix.evaluate(AbstractMatrix1Matrix.java:31)
    at org.matheclipse.core.eval.EvalEngine.evalASTBuiltinFunction(EvalEngine.java:906)
  public void testSystem081() {
    check(
        "Inverse(s*{{1,0,0},{0,1,0},{0,0,1}}-{{-1,1,1},{-4,-4,1},{1,1,1}})",
        "{{(5-3*s-s^2)/(10-s-4*s^2-s^3),s/(-10+s+4*s^2+s^3),(5+s)/(-10+s+4*s^2+s^3)},\n"
            + " {(5-4*s)/(-10+s+4*s^2+s^3),(2-s^2)/(10-s-4*s^2-s^3),(-3+s)/(-10+s+4*s^2+s^3)},\n"
            + " {-s/(10-s-4*s^2-s^3),(-2-s)/(10-s-4*s^2-s^3),(8+5*s+s^2)/(-10+s+4*s^2+s^3)}}");

  }
axkr commented 3 years ago

Hard to describe exactly, but the LUDecomposition seems to have more complicated (rational) elements now?

Example: LUDecomposition({{1, 2, 3}, {3, 4, 11}, {13, 7, 8}})

Before:

{
{{1,0,0},
 {3,1,0},
 {13,19/2,1}},
{{1,2,3},
 {0,-2,2},
 {0,0,-50}},{1,2,3}}

After:

{
{{1,0,0},
 {3/13,1,0},
 {1/13,19/31,1}},
{{13,7,8},
 {0,31/13,119/13},
 {0,0,-100/31}},{3,2,1}}
axkr commented 3 years ago

As you design a 2.0 release. Does it make sense to mix upper- and lower‐triangular matrices in only one "result matrix" to safe memory?

See:

BryanCazabonne commented 3 years ago

I will investigate this.

The regressions you see are related to this issue and this pull request. I'm just surprised the see different results for the LUDecomposition class.

BryanCazabonne commented 3 years ago

Hard to describe exactly, but the LUDecomposition seems to have more complicated (rational) elements now?

I'm very surprised about this observation. Last change in LUDecomposition class occurred 2 years ago.

Can we have an option to skip this new feature or a dedicated method which returns the largest element.

I'm thinking about the best solution for your problem. Unfortunately, I don't have one yet. If it's a real problem for you, maybe we can revert the commit. I don't know.

axkr commented 3 years ago

With this patch in FieldLUDecomposition the symbolic implementation can be "switched on/off". If numericPermutationChoice == false the "old behaviour" works at least in my JUnit test cases.

diff --git a/hipparchus-core/src/main/java/org/hipparchus/linear/FieldLUDecomposition.java b/hipparchus-core/src/main/java/org/hipparchus/linear/FieldLUDecomposition.java
index 7ef21cb..d6c7cac 100644
--- a/hipparchus-core/src/main/java/org/hipparchus/linear/FieldLUDecomposition.java
+++ b/hipparchus-core/src/main/java/org/hipparchus/linear/FieldLUDecomposition.java
@@ -89,13 +89,17 @@
         this(matrix, e -> e.isZero());
     }

+    public FieldLUDecomposition(FieldMatrix<T> matrix, final Predicate<T> zeroChecker ) {
+       this(matrix, zeroChecker, true);
+    }
     /**
      * Calculates the LU-decomposition of the given matrix.
      * @param matrix The matrix to decompose.
      * @param zeroChecker checker for zero elements
+     * @param numericPermutationChoice if <code>true</code> choose permutation index with numeric calculations, otherwise choose with <code>zeroChecker</code>
      * @throws MathIllegalArgumentException if matrix is not square
      */
-    public FieldLUDecomposition(FieldMatrix<T> matrix, final Predicate<T> zeroChecker) {
+    public FieldLUDecomposition(FieldMatrix<T> matrix, final Predicate<T> zeroChecker, boolean numericPermutationChoice) {
         if (!matrix.isSquare()) {
             throw new MathIllegalArgumentException(LocalizedCoreFormats.NON_SQUARE_MATRIX,
                                                    matrix.getRowDimension(), matrix.getColumnDimension());
@@ -128,9 +132,10 @@
                 }
                 luRow[col] = sum;
             }
-
-            // lower
+ 
             int max = col; // permutation row
+            if (numericPermutationChoice) {
+               // lower
             double largest = Double.NEGATIVE_INFINITY;
             for (int row = col; row < m; row++) {
                 final T[] luRow = lu[row];
@@ -148,6 +153,24 @@
                 }
             }

+            } else {
+               // lower
+                int nonZero = col; // permutation row
+                for (int row = col; row < m; row++) {
+                    final T[] luRow = lu[row];
+                    T sum = luRow[col];
+                    for (int i = 0; i < col; i++) {
+                        sum = sum.subtract(luRow[i].multiply(lu[i][col]));
+                    }
+                    luRow[col] = sum;
+
+                    if (zeroChecker.test(lu[nonZero][col])) {
+                        // try to select a better permutation choice
+                        ++nonZero;
+                    }
+                }
+                max = nonZero;
+            }
             // Singularity check
             if (zeroChecker.test(lu[max][col])) {
                 singular = true;
BryanCazabonne commented 3 years ago

I just merged you patch into master branch. Do you think we can close this issue?

axkr commented 3 years ago

Yes I close this issue

axkr commented 3 years ago

Are there Maven snapshot JARs available with the change?

BryanCazabonne commented 3 years ago

If you have a pom.xml file in your project, you can directly put 2.0-SNAPSHOT in the hipparchus version number

Otherwise, maybe in the following link: https://packages.orekit.org/#browse/browse:maven-public:org%2Fhipparchus%2Fhipparchus-aggregator%2F2.0-SNAPSHOT%2F2.0-20210604.095007-28

Please note that this link is for the hipparchus-aggregator, but other hipparchus packages are also available.

axkr commented 3 years ago

Yes but your latest commit isn't included?

BryanCazabonne commented 3 years ago

Ok, I looked the sources in the artifacts and the fixed is not included. I have to look at why.

axkr commented 3 years ago

The snapshot is available now.

BryanCazabonne commented 3 years ago

Great! I saw that Luc pushed some code yesterday so I think it updated the Snapshot.

axkr commented 3 years ago

Yes everything's ok