Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3
    • Labels:
      None

      Description

      This should be a very easy feature to add and has many applications.

      1. MATH-1004_updated.diff
        4 kB
        Thomas Neidhart
      2. MATH-1004.patch
        6 kB
        Thomas Neidhart

        Issue Links

          Activity

          Hide
          Gilles added a comment -

          Patch welcome.
          Please note that there exists an extensive issue report about refactoring the CM matrix (and "vector") API.

          Show
          Gilles added a comment - Patch welcome. Please note that there exists an extensive issue report about refactoring the CM matrix (and "vector") API.
          Hide
          Thomas Neidhart added a comment -

          Attached a patch with the following changes:

          • added MatrixUtils.inverse(DiagonalMatrix)
          • added MathArrays.checkNonZero(double[])
          • added unit tests

          I think MatrixUtils is a good place for this new function as there is already a blockInverse method.

          Show
          Thomas Neidhart added a comment - Attached a patch with the following changes: added MatrixUtils.inverse(DiagonalMatrix) added MathArrays.checkNonZero(double[]) added unit tests I think MatrixUtils is a good place for this new function as there is already a blockInverse method.
          Hide
          Thomas Neidhart added a comment - - edited

          We might also, for convenience, add a MatrixUtils.inverse(RealMatrix) method which delegates to the inverse(DiagonalMatrix) variant in case of a DiagonalMatrix, and in other cases use a LU or CholeskyDecomposition to compute the inverse, depending on the properties of the matrix.

          Show
          Thomas Neidhart added a comment - - edited We might also, for convenience, add a MatrixUtils.inverse(RealMatrix) method which delegates to the inverse(DiagonalMatrix) variant in case of a DiagonalMatrix, and in other cases use a LU or CholeskyDecomposition to compute the inverse, depending on the properties of the matrix.
          Hide
          Thomas Neidhart added a comment -

          DiagonalMatrix already has a few additional public method, e.g. add(DiagonalMatrix), so it would probably also be ok to add a inverse() method to the class itself.

          Any opinions on this topic?

          Imho, I would prefer adding it as MatrixUtils.inverse(DiagonalMatrix).

          Show
          Thomas Neidhart added a comment - DiagonalMatrix already has a few additional public method, e.g. add(DiagonalMatrix), so it would probably also be ok to add a inverse() method to the class itself. Any opinions on this topic? Imho, I would prefer adding it as MatrixUtils.inverse(DiagonalMatrix).
          Hide
          Gilles added a comment -

          I would prefer adding it as MatrixUtils.inverse(DiagonalMatrix)

          Why?
          Note that the "blockInverse" operates on any base type that can provide an inverse. It's (sort-of) higher-level. Here the implementation is specific to the actual type.

          Show
          Gilles added a comment - I would prefer adding it as MatrixUtils.inverse(DiagonalMatrix) Why? Note that the "blockInverse" operates on any base type that can provide an inverse. It's (sort-of) higher-level. Here the implementation is specific to the actual type.
          Hide
          Phil Steitz added a comment -

          I would favor adding MatrixUtils.inverse(RealMatrix) and have it just do the instanceOf check, delegating or inlining the specialized version. I am ambivalent as to whether or not to add inverse(DiagonalMatrix) to the public API.

          Show
          Phil Steitz added a comment - I would favor adding MatrixUtils.inverse(RealMatrix) and have it just do the instanceOf check, delegating or inlining the specialized version. I am ambivalent as to whether or not to add inverse(DiagonalMatrix) to the public API.
          Hide
          Thomas Neidhart added a comment - - edited

          Ok, so then let's add a DiagonalMatrix.inverse() method.

          But I would also suggest to add a method with an epsilon to check for a singular matrix:

          • DiagonalMatrix inverse() -> uses default epsilon, e.g. 1e-12
          • DiagonalMatrix inverse(double epsilon)
          • boolean isSingular(double epsilon)
          Show
          Thomas Neidhart added a comment - - edited Ok, so then let's add a DiagonalMatrix.inverse() method. But I would also suggest to add a method with an epsilon to check for a singular matrix: DiagonalMatrix inverse() -> uses default epsilon, e.g. 1e-12 DiagonalMatrix inverse(double epsilon) boolean isSingular(double epsilon)
          Hide
          Gilles added a comment -

          [...] adding MatrixUtils.inverse(RealMatrix) and have it just do the instanceOf check [...]

          Do you mean that we would forbid users to call this method on an invertible matrix that happens to not be of the "DiagonalMatrix" type?

          [...] whether or not to add inverse(DiagonalMatrix) to the public API.

          IMHO, we should avoid crowding "XxxUtils" classes. IOW, there should be a good reason why the method cannot be somewhere else.

          Show
          Gilles added a comment - [...] adding MatrixUtils.inverse(RealMatrix) and have it just do the instanceOf check [...] Do you mean that we would forbid users to call this method on an invertible matrix that happens to not be of the "DiagonalMatrix" type? [...] whether or not to add inverse(DiagonalMatrix) to the public API. IMHO, we should avoid crowding "XxxUtils" classes. IOW, there should be a good reason why the method cannot be somewhere else.
          Hide
          Phil Steitz added a comment -

          Sorry, Gilles, I was not clear enough. What I agreeing with was the proposal to add an inverse(RealMatrix) method to MatrixUtils that would handle any RealMatrix, throwing only if the matrix is singular or non-square. This method would essentially replace what used to be in RealMatrix itself, but which users now have to get a decomposition solver to do, or use the block matrix version you reference. It would include a default solver-based implementation (which one?), but first check if the matrix is diagonal (using instanceOf) and if so, inline or delegate to the impl for diagonal matrices.

          I agree with you that we should not clutter the Utils classes, which is why I was ambivalent on exposing the DiagonalMatrix version itself. I think it is a good idea though to make a general inverse method available so people just wanting an inverse don't have to search around to figure out the decomp solver API. I know the blockInverse method is already there; but it is really a specialized implementation and requires an additional parameter, so I would be +1 to add a simple, general-purpose method.

          Show
          Phil Steitz added a comment - Sorry, Gilles, I was not clear enough. What I agreeing with was the proposal to add an inverse(RealMatrix) method to MatrixUtils that would handle any RealMatrix, throwing only if the matrix is singular or non-square. This method would essentially replace what used to be in RealMatrix itself, but which users now have to get a decomposition solver to do, or use the block matrix version you reference. It would include a default solver-based implementation (which one?), but first check if the matrix is diagonal (using instanceOf) and if so, inline or delegate to the impl for diagonal matrices. I agree with you that we should not clutter the Utils classes, which is why I was ambivalent on exposing the DiagonalMatrix version itself. I think it is a good idea though to make a general inverse method available so people just wanting an inverse don't have to search around to figure out the decomp solver API. I know the blockInverse method is already there; but it is really a specialized implementation and requires an additional parameter, so I would be +1 to add a simple, general-purpose method.
          Hide
          Gilles added a comment -

          OK, I got it now.

          I know the blockInverse method is already there [...]

          I take this opportunity to remind that the usefulness of "blockInverse" is still to be demonstrated (cf. comment in the unit test file).

          Show
          Gilles added a comment - OK, I got it now. I know the blockInverse method is already there [...] I take this opportunity to remind that the usefulness of "blockInverse" is still to be demonstrated (cf. comment in the unit test file).
          Hide
          Phil Steitz added a comment -

          +1 to Thomas' suggestion to have DiagonalMatrix directly implement inverse (with configurable singularity threshold).

          Probably should be a separate ticket, but +1 as well to MatrixUtils.inverse. Question there is what is best generic use decomp solver for inversion.

          Show
          Phil Steitz added a comment - +1 to Thomas' suggestion to have DiagonalMatrix directly implement inverse (with configurable singularity threshold). Probably should be a separate ticket, but +1 as well to MatrixUtils.inverse. Question there is what is best generic use decomp solver for inversion.
          Hide
          Thomas Neidhart added a comment -

          Ok I will update the patch with all the suggestions.

          Show
          Thomas Neidhart added a comment - Ok I will update the patch with all the suggestions.
          Hide
          Thomas Neidhart added a comment -

          Attached an updated patch.

          I decided to use QRDecomposition which is supposed to be more stable than LUDecomposition.

          The patch does not yet include the unit tests from the other patch, but if this patch is accepted, I will merge them together.

          Show
          Thomas Neidhart added a comment - Attached an updated patch. I decided to use QRDecomposition which is supposed to be more stable than LUDecomposition. The patch does not yet include the unit tests from the other patch, but if this patch is accepted, I will merge them together.
          Hide
          Phil Steitz added a comment -

          Looks good, modulo one thing. I can't tell under what conditions exactly this thing is going to throw DimensionMismatchException (which looks like the only thing it can throw). In particular, it is not clear to me what this thing does with a non-square matrix. Looks like it will try to get the pseudo-inverse. Or does it throw? If the advertised exceptions are all that the method can throw, great; if not it would be good to add whatever else it can throw. If it does handle non-square matrices, we should say "(or pseudo-inverse)" and ideally provide a reference to what exactly that means in the javadoc; if not we should check argument and throw some kind of IAE (probably not propagate the confusing DME). This doco / arg validation gap should probably be closed in the decomposition solvers as well.

          Show
          Phil Steitz added a comment - Looks good, modulo one thing. I can't tell under what conditions exactly this thing is going to throw DimensionMismatchException (which looks like the only thing it can throw). In particular, it is not clear to me what this thing does with a non-square matrix. Looks like it will try to get the pseudo-inverse. Or does it throw? If the advertised exceptions are all that the method can throw, great; if not it would be good to add whatever else it can throw. If it does handle non-square matrices, we should say "(or pseudo-inverse)" and ideally provide a reference to what exactly that means in the javadoc; if not we should check argument and throw some kind of IAE (probably not propagate the confusing DME). This doco / arg validation gap should probably be closed in the decomposition solvers as well.
          Hide
          Thomas Neidhart added a comment -

          Yes you are right, I forgot about this case. Imho we should explicitly check for a non-square matrix and throw a MatrixDimensionMismatchException.

          And also update the javadoc of the solvers as needed as you suggest.

          Show
          Thomas Neidhart added a comment - Yes you are right, I forgot about this case. Imho we should explicitly check for a non-square matrix and throw a MatrixDimensionMismatchException. And also update the javadoc of the solvers as needed as you suggest.
          Hide
          Thomas Neidhart added a comment -

          Applied patch with additional changes and unit tests in r1533638:

          • throw a NonSquareMatrixException in case a non-square matrix is provided
          Show
          Thomas Neidhart added a comment - Applied patch with additional changes and unit tests in r1533638: throw a NonSquareMatrixException in case a non-square matrix is provided

            People

            • Assignee:
              Thomas Neidhart
              Reporter:
              Ajo Fod
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development