Improve precondition checks (#62)

* Refactor argument checking; Introduce new unsafeSetup method

* Fix tests

* Fix bug

* Add test with verbose true

Author: atraplet

src/main/java/com/ustermetrics/ecos4j/Model.java

@@ -10,11 +10,13 @@
 import java.lang.foreign.MemorySegment;
 import java.util.Arrays;
 import java.util.Optional;
+import java.util.stream.IntStream;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Verify.verify;
 import static com.ustermetrics.ecos4j.bindings.ecos_h.*;
+import static java.lang.Math.toIntExact;
 import static java.lang.foreign.MemorySegment.NULL;
 
 /**
@@ -74,70 +76,60 @@ public static String version() {
     public void setup(long l, long @NonNull [] q, long nExC, double @NonNull [] gpr, long @NonNull [] gjc,
                       long @NonNull [] gir, double @NonNull [] c, double @NonNull [] h, double[] apr, long[] ajc,
                       long[] air, double[] b) {
-        checkState(stage == Stage.NEW, "Model must be in stage new");
-
-        checkArgument(l >= 0, "dimension of the positive orthant l must be non-negative");
-        val nCones = q.length;
-        checkArgument(nCones == 0 || Arrays.stream(q).allMatch(d -> d > 0),
-                "second-order cone dimensions q must be empty or each dimension q[i] must be positive");
-        checkArgument(nExC >= 0, "number of exponential cones nExC must be non-negative");
-        val nnzG = gpr.length;
-        checkArgument(nnzG > 0, "number of non-zero elements in G (gpr.length) must be positive");
-        checkArgument(nnzG == gir.length,
-                "number of non-zero elements in G (gpr.length) must be equal to the number of elements in the row " +
-                        "index of G (gir.length)");
-        val nColsG = gjc.length - 1;
-        checkArgument(nColsG > 0, "number of columns of G (gjc.length - 1) must be positive");
-        n = c.length;
-        checkArgument(n > 0, "number of variables x (c.length) must be positive");
-        m = h.length;
-        checkArgument(m > 0, "dimension of all cones (h.length) must be positive");
-        checkArgument(m == l + Arrays.stream(q).sum() + 3 * nExC,
-                "dimension of all cones (h.length) must be equal to the sum of the positive orthant dimension l, the " +
-                        "second-order cone dimensions q[i], and three times the number of exponential cones 3 * nExC");
-        checkArgument(nColsG == n, "number of columns of G (gjc.length - 1) must be equal to the number of variables " +
-                "x (c.length)");
+        checkArguments(l, q, nExC, gpr, gjc, gir, c, h, apr, ajc, air, b);
+        unsafeSetup(l, q, nExC, gpr, gjc, gir, c, h, apr, ajc, air, b);
+    }
 
+    private static void checkArguments(long l, long[] q, long nExC, double[] gpr, long[] gjc, long[] gir, double[] c,
+                                       double[] h, double[] apr, long[] ajc, long[] air, double[] b) {
+        checkCone(l, q, nExC, h);
+        checkMatrix(gpr, gjc, gir, h.length, c.length, "G");
         checkArgument(apr != null && ajc != null && air != null && b != null || apr == null && ajc == null && air == null && b == null,
-                "A (apr, ajc, air) and b must be supplied (all non-null) or omitted (all null) together");
+                "all arguments of the equalities must be supplied together or must be null together");
         if (apr != null) {
-            val nnzA = apr.length;
-            checkArgument(nnzA > 0, "number of non-zero elements in A (apr.length) must be positive");
-            checkArgument(nnzA == air.length,
-                    "number of non-zero elements in A (apr.length) must be equal to the number of elements in the row" +
-                            " index of A (air.length)");
-            val nColsA = ajc.length - 1;
-            checkArgument(nColsA > 0, "number of columns of A (ajc.length - 1) must be positive");
-            p = b.length;
-            checkArgument(p > 0, "number of equalities (b.length) must be positive");
-            checkArgument(nColsA == n, "number of columns of A (ajc.length - 1) must be equal to the number of " +
-                    "variables x (c.length)");
-        } else {
-            p = 0;
+            checkMatrix(apr, ajc, air, b.length, c.length, "A");
         }
+    }
 
-        val qSeg = arena.allocateFrom(C_LONG_LONG, q);
-        val gprSeg = arena.allocateFrom(C_DOUBLE, gpr);
-        val gjcSeg = arena.allocateFrom(C_LONG_LONG, gjc);
-        val girSeg = arena.allocateFrom(C_LONG_LONG, gir);
-        val aprSeg = apr != null ? arena.allocateFrom(C_DOUBLE, apr) : NULL;
-        val ajcSeg = ajc != null ? arena.allocateFrom(C_LONG_LONG, ajc) : NULL;
-        val airSeg = air != null ? arena.allocateFrom(C_LONG_LONG, air) : NULL;
-        val cSeg = arena.allocateFrom(C_DOUBLE, c);
-        val hSeg = arena.allocateFrom(C_DOUBLE, h);
-        val bSeg = b != null ? arena.allocateFrom(C_DOUBLE, b) : NULL;
-
-        workSeg = ECOS_setup(n, m, p, l, nCones, qSeg, nExC, gprSeg, gjcSeg, girSeg, aprSeg, ajcSeg, airSeg, cSeg,
-                hSeg, bSeg).reinterpret(pwork.sizeof(), arena, null);
-        verify(!NULL.equals(workSeg));
-
-        stgsSeg = pwork.stgs(workSeg).reinterpret(settings.sizeof(), arena, null);
-        infoSeg = pwork.info(workSeg).reinterpret(stats.sizeof(), arena, null);
+    private static void checkCone(long l, long[] q, long nExC, double[] h) {
+        checkArgument(l >= 0, "dimension of the positive orthant must be non-negative");
+        checkArgument(q.length == 0 || Arrays.stream(q).allMatch(i -> i > 0),
+                "second-order cone dimensions must have zero length or each dimension must be positive");
+        checkArgument(nExC >= 0, "number of exponential cones must be non-negative");
+        checkArgument(h.length == l + Arrays.stream(q).sum() + 3 * nExC,
+                "dimension of the convex cone K must be equal to the sum of the positive orthant dimension, the " +
+                        "second-order cone dimensions, and three times the number of exponential cones");
+    }
 
-        stage = Stage.SETUP;
+    private static void checkMatrix(double[] mpr, long[] mjc, long[] mir, int nRows, int nCols, String mName) {
+        checkArgument(nRows > 0, "matrix %s: number of rows must be positive", mName);
+        checkArgument(nCols > 0, "matrix %s: number of columns must be positive", mName);
+        val nnz = mpr.length;
+        checkArgument(0 < nnz && nnz <= nRows * nCols,
+                "matrix %s: number of non-zero entries must be greater than zero and less equal than the number of " +
+                        "rows times the number of columns", mName);
+        checkArgument(mir.length == nnz,
+                "matrix %s: number of entries in the row index must be equal to the number of non-zero entries",
+                mName);
+        checkArgument(mjc.length == nCols + 1,
+                "length of the column index must be equal to the number of columns plus one", mName);
+        checkArgument(Arrays.stream(mir).allMatch(i -> 0 <= i && i < nRows),
+                "matrix %s: entries of the row index must be greater equal zero and less than the number of rows",
+                mName);
+        checkArgument(mjc[0] == 0 && mjc[mjc.length - 1] == nnz,
+                "matrix %s: the first entry of the column index must be equal to zero and the last entry must be " +
+                        "equal to the number of non-zero entries", mName);
+        checkArgument(IntStream.range(0, mjc.length - 1).allMatch(i ->
+                        0 <= mjc[i] && mjc[i] <= nnz && mjc[i] <= mjc[i + 1]
+                                && IntStream.range(toIntExact(mjc[i]), toIntExact(mjc[i + 1]) - 1).allMatch(j -> mir[j] < mir[j + 1])),
+                "matrix %s: entries of the column index must be greater equal zero, less equal than the number of " +
+                        "non-zero entries, and must be ordered, entries of the row index within each column must be " +
+                        "strictly ordered", mName);
     }
 
     /**
+     * Set up the {@link Model} data.
+     * <p>
      * Same as
      * {@link Model#setup(long l, long[] q, long nExC, double[] gpr, long[] gjc, long[] gir, double[] c, double[] h, double[] apr, long[] ajc, long[] air, double[] b)}
      * without equality constraint, i.e. {@code apr}, {@code ajc}, {@code air}, and {@code b} are empty arrays.
@@ -156,6 +148,60 @@ public void setup(long l, long @NonNull [] q, long nExC, double @NonNull [] gpr,
         setup(l, q, nExC, gpr, gjc, gir, c, h, null, null, null, null);
     }
 
+    /**
+     * Unsafe set up the {@link Model} data.
+     * <p>
+     * Same as
+     * {@link Model#setup(long l, long[] q, long nExC, double[] gpr, long[] gjc, long[] gir, double[] c, double[] h, double[] apr, long[] ajc, long[] air, double[] b)}
+     * without any precondition checks on its arguments.
+     * <p>
+     * <b>Warning: Setting the arguments incorrectly may lead to incorrect results in the best case. In the worst
+     * case, it can crash the JVM and may silently result in memory corruption.</b>
+     *
+     * @param l    the dimension of the positive orthant.
+     * @param q    the dimensions of the second-order cones.
+     * @param nExC the number of exponential cones.
+     * @param gpr  the sparse G matrix data (Column Compressed Storage CCS).
+     * @param gjc  the sparse G matrix column index (CCS).
+     * @param gir  the sparse G matrix row index (CCS).
+     * @param c    the cost function weights.
+     * @param h    the right-hand-side of the cone constraints.
+     * @param apr  the (optional) sparse A matrix data (CCS).
+     * @param ajc  the (optional) sparse A matrix column index (CCS).
+     * @param air  the (optional) sparse A matrix row index (CCS).
+     * @param b    the (optional) right-hand-side of the equalities.
+     */
+    public void unsafeSetup(long l, long @NonNull [] q, long nExC, double @NonNull [] gpr, long @NonNull [] gjc,
+                            long @NonNull [] gir, double @NonNull [] c, double @NonNull [] h, double[] apr, long[] ajc,
+                            long[] air, double[] b) {
+        checkState(stage == Stage.NEW, "model must be in stage new");
+
+        n = c.length;
+        m = h.length;
+        p = apr != null ? b.length : 0;
+        val nCones = q.length;
+
+        val qSeg = arena.allocateFrom(C_LONG_LONG, q);
+        val gprSeg = arena.allocateFrom(C_DOUBLE, gpr);
+        val gjcSeg = arena.allocateFrom(C_LONG_LONG, gjc);
+        val girSeg = arena.allocateFrom(C_LONG_LONG, gir);
+        val aprSeg = apr != null ? arena.allocateFrom(C_DOUBLE, apr) : NULL;
+        val ajcSeg = ajc != null ? arena.allocateFrom(C_LONG_LONG, ajc) : NULL;
+        val airSeg = air != null ? arena.allocateFrom(C_LONG_LONG, air) : NULL;
+        val cSeg = arena.allocateFrom(C_DOUBLE, c);
+        val hSeg = arena.allocateFrom(C_DOUBLE, h);
+        val bSeg = b != null ? arena.allocateFrom(C_DOUBLE, b) : NULL;
+
+        workSeg = ECOS_setup(n, m, p, l, nCones, qSeg, nExC, gprSeg, gjcSeg, girSeg, aprSeg, ajcSeg, airSeg, cSeg,
+                hSeg, bSeg).reinterpret(pwork.sizeof(), arena, null);
+        verify(!NULL.equals(workSeg));
+
+        stgsSeg = pwork.stgs(workSeg).reinterpret(settings.sizeof(), arena, null);
+        infoSeg = pwork.info(workSeg).reinterpret(stats.sizeof(), arena, null);
+
+        stage = Stage.SETUP;
+    }
+
     /**
      * Sets the <a href="https://github.com/embotech/ecos">ECOS</a> solver options.
      * <p>
@@ -164,7 +210,7 @@ public void setup(long l, long @NonNull [] q, long nExC, double @NonNull [] gpr,
      * @param parameters the parameter object for the solver options.
      */
     public void setParameters(@NonNull Parameters parameters) {
-        checkState(stage != Stage.NEW, "Model must not be in stage new");
+        checkState(stage != Stage.NEW, "model must not be in stage new");
 
         Optional.ofNullable(parameters.feasTol())
                 .ifPresent(feasTol -> settings.feastol(stgsSeg, feasTol));
@@ -192,7 +238,7 @@ public void setParameters(@NonNull Parameters parameters) {
      * @return the solver status.
      */
     public Status optimize() {
-        checkState(stage != Stage.NEW, "Model must not be in stage new");
+        checkState(stage != Stage.NEW, "model must not be in stage new");
 
         val status = ECOS_solve(workSeg);
         if (settings.verbose(stgsSeg) == 1) {
@@ -209,7 +255,7 @@ public Status optimize() {
      * Cleanup: free this {@link Model} native memory.
      */
     public void cleanup() {
-        checkState(stage != Stage.NEW, "Model must not be in stage new");
+        checkState(stage != Stage.NEW, "model must not be in stage new");
         ECOS_cleanup(workSeg, 0);
         stage = Stage.NEW;
     }
@@ -349,6 +395,11 @@ public long iter() {
         return pwork.s(workSeg).reinterpret(C_DOUBLE.byteSize() * m, arena, null).toArray(C_DOUBLE);
     }
 
+    private void checkStageIsOptimizedAndStatusIsNotFatal() {
+        checkState(stage == Stage.OPTIMIZED, "model must be in stage optimized");
+        checkState(status != Status.FATAL, "solver status must not be fatal");
+    }
+
     @Override
     public void close() {
         if (stage != Stage.NEW) {
@@ -357,9 +408,4 @@ public void close() {
         arena.close();
     }
 
-    private void checkStageIsOptimizedAndStatusIsNotFatal() {
-        checkState(stage == Stage.OPTIMIZED, "Model must be in stage optimized");
-        checkState(status != Status.FATAL, "Solver status must not be fatal");
-    }
-
 }

src/test/java/com/ustermetrics/ecos4j/ModelTest.java

@@ -77,6 +77,21 @@ void solvePortfolioOptimizationProblemReturnsExpectedSolution() {
         }
     }
 
+    @Test
+    void solvePortfolioOptimizationProblemWithVerboseParameterTrueReturnsOptimal() {
+        try (val model = new Model()) {
+            model.setup(l, q, nExC, gpr, gjc, gir, c, h, apr, ajc, air, b);
+            val parameters = Parameters.builder()
+                    .verbose(true)
+                    .build();
+            model.setParameters(parameters);
+
+            val status = model.optimize();
+
+            assertEquals(Status.OPTIMAL, status);
+        }
+    }
+
     @Test
     void solveModifiedPortfolioOptimizationProblemWithMaxitLimitReturnsMaxitStatus() {
         try (val model = new Model()) {
@@ -102,7 +117,7 @@ void optimizeWithoutSetupThrowsException() {
             }
         });
 
-        assertEquals("Model must not be in stage new", exception.getMessage());
+        assertEquals("model must not be in stage new", exception.getMessage());
     }
 
     @Test
@@ -113,7 +128,7 @@ void getPrimalVariablesWithoutSetupThrowsException() {
             }
         });
 
-        assertEquals("Model must be in stage optimized", exception.getMessage());
+        assertEquals("model must be in stage optimized", exception.getMessage());
     }
 
     @Test
@@ -124,7 +139,7 @@ void cleanupWithoutSetupThrowsException() {
             }
         });
 
-        assertEquals("Model must not be in stage new", exception.getMessage());
+        assertEquals("model must not be in stage new", exception.getMessage());
     }
 
     @Test
@@ -135,7 +150,7 @@ void setParametersWithoutSetupThrowsException() {
             }
         });
 
-        assertEquals("Model must not be in stage new", exception.getMessage());
+        assertEquals("model must not be in stage new", exception.getMessage());
     }
 
     @Test
@@ -149,7 +164,7 @@ void setupAfterOptimizeThrowsException() {
             }
         });
 
-        assertEquals("Model must be in stage new", exception.getMessage());
+        assertEquals("model must be in stage new", exception.getMessage());
     }
 
     @Test
@@ -160,7 +175,7 @@ void setupWithInvalidPositiveOrthantDimensionThrowsException() {
             }
         });
 
-        assertEquals("dimension of the positive orthant l must be non-negative", exception.getMessage());
+        assertEquals("dimension of the positive orthant must be non-negative", exception.getMessage());
     }
 
     @Test
@@ -171,7 +186,7 @@ void setupWithInvalidNumberOfExponentialConesThrowsException() {
             }
         });
 
-        assertEquals("number of exponential cones nExC must be non-negative", exception.getMessage());
+        assertEquals("number of exponential cones must be non-negative", exception.getMessage());
     }
 
     @Test