Commons Math
  1. Commons Math
  2. MATH-653

Rename "AbstractRealVector" to "RealVector"

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Labels:
      None

      Description

      "AbstractRealVector" is the only implementation of the "RealVector" interface.

        Activity

        Hide
        Sébastien Brisard added a comment -
        • EigenDecompositionImpl done with in rev. 1165505
        • LUDecompositionImpl in rev. 1165506
        • QRDecompositionSolver in rev. 1165507
        • SingularValueDecompositionSolver in rev. 1165508
        Show
        Sébastien Brisard added a comment - EigenDecompositionImpl done with in rev. 1165505 LUDecompositionImpl in rev. 1165506 QRDecompositionSolver in rev. 1165507 SingularValueDecompositionSolver in rev. 1165508
        Hide
        Sébastien Brisard added a comment -

        Updated CholeskyDecompositionImpl in rev. 1165155

        Show
        Sébastien Brisard added a comment - Updated CholeskyDecompositionImpl in rev. 1165155
        Hide
        Sébastien Brisard added a comment - - edited
        • In rev. 1164618: removed references do DecompositionSolver.solve(double[]) from all unit tests of RealMatrix implementations.
        • In rev. 1164622: same with unit tests of DecompositionSolver implementations.
        Show
        Sébastien Brisard added a comment - - edited In rev. 1164618: removed references do DecompositionSolver.solve(double[]) from all unit tests of RealMatrix implementations. In rev. 1164622: same with unit tests of DecompositionSolver implementations.
        Hide
        Sébastien Brisard added a comment -

        Done in rev 1164615.

        Show
        Sébastien Brisard added a comment - Done in rev 1164615.
        Hide
        Luc Maisonobe added a comment -

        OK from me.

        Show
        Luc Maisonobe added a comment - OK from me.
        Hide
        Sébastien Brisard added a comment -

        OK, I've started to look into the DecompositionSolver. Unfortunately, the method solve(double[]) gets called in GaussNewtonOptimizer. This class makes thorough use of double[] as a representation of vectors. There might be a gain in replacing occurences of double[] by ArrayRealVector. This might be like opening Pandora's box, though.
        For the time being, I can propose a quick fix for GaussNewtonOptimizer to call solve(RealVector) instead of solve(double[]), and work on all implementations of DecompositionSolver so as to remove solve(double[]), and keep only solve(RealVector) (and possibly solve(ArrayRealVector)).

        How do you like that?

        Show
        Sébastien Brisard added a comment - OK, I've started to look into the DecompositionSolver . Unfortunately, the method solve(double[]) gets called in GaussNewtonOptimizer . This class makes thorough use of double[] as a representation of vectors. There might be a gain in replacing occurences of double[] by ArrayRealVector . This might be like opening Pandora's box, though. For the time being, I can propose a quick fix for GaussNewtonOptimizer to call solve(RealVector) instead of solve(double[]) , and work on all implementations of DecompositionSolver so as to remove solve(double[]) , and keep only solve(RealVector) (and possibly solve(ArrayRealVector) ). How do you like that?
        Hide
        Sébastien Brisard added a comment -

        I can take care of RealLinearOperator [...]

        Done in revision 1163515 (took me some time to find out how to commit). As agreed, I left RealMatrix untouched (had to remove a @Override tag).

        Show
        Sébastien Brisard added a comment - I can take care of RealLinearOperator [...] Done in revision 1163515 (took me some time to find out how to commit). As agreed, I left RealMatrix untouched (had to remove a @Override tag).
        Hide
        Gilles added a comment -

        I would favor inlining.

        Done in revision 1162800.

        I can take care of RealLinearOperator [...]

        OK.

        any plan to rename AbstractRealMatrix to RealMatrix?

        That certainly should be done.
        Maybe better wait for the conclusion of the ongoing discussion on what to remove/rename in that interface/class...

        DecompositionSolver [...] remove any methods taking double[] as an input.

        You are welcome to do it.

        Show
        Gilles added a comment - I would favor inlining. Done in revision 1162800. I can take care of RealLinearOperator [...] OK. any plan to rename AbstractRealMatrix to RealMatrix? That certainly should be done. Maybe better wait for the conclusion of the ongoing discussion on what to remove/rename in that interface/class... DecompositionSolver [...] remove any methods taking double[] as an input. You are welcome to do it.
        Hide
        Sébastien Brisard added a comment -

        Gilles,
        I would favor inlining.
        I'm still working on iterative solvers, so if you want, I can take care of RealLinearOperator (which was added for this special purpose). Indeed, following what you've just done double[] operate(double[] x) should probably be removed from this class. Question is: should it also disappear from the daughter class AbstractRealMatrix? As an aside, is there any plan to rename AbstractRealMatrix to RealMatrix?
        Since I'm still working on the interface of linear solvers, I'll remove the method solve(RealLinearOperator, double[], double[], and keep only solve(RealLinearOperator, RealVector, RealVector before I submit anything.
        I guess the next step (setting apart the refactoring of RealMatrix/AbstractRealMatrix in the same fashion) would be to look into DecompositionSolver as well, and remove any methods taking double[] as an input.

        Show
        Sébastien Brisard added a comment - Gilles, I would favor inlining. I'm still working on iterative solvers, so if you want, I can take care of RealLinearOperator (which was added for this special purpose). Indeed, following what you've just done double[] operate(double[] x) should probably be removed from this class. Question is: should it also disappear from the daughter class AbstractRealMatrix ? As an aside, is there any plan to rename AbstractRealMatrix to RealMatrix ? Since I'm still working on the interface of linear solvers, I'll remove the method solve(RealLinearOperator, double[], double[] , and keep only solve(RealLinearOperator, RealVector, RealVector before I submit anything. I guess the next step (setting apart the refactoring of RealMatrix/AbstractRealMatrix in the same fashion) would be to look into DecompositionSolver as well, and remove any methods taking double[] as an input.
        Hide
        Gilles added a comment -

        Removed methods taking a "double[]" arguments from "RealVector" (revision 1162629).

        I've left them in "ArrayRealVector"; they seem to make sense there because the "double[]" is the underlying storage of that class.
        Also, they are called within methods that override the base class ones. However, if it is preferable that subclasses do not contain methods that are not in the base class interface, we can either

        • inline the specialized code and remove the methods, or
        • make those methods private.

        What do you think?

        Show
        Gilles added a comment - Removed methods taking a "double[]" arguments from "RealVector" (revision 1162629). I've left them in "ArrayRealVector"; they seem to make sense there because the "double[]" is the underlying storage of that class. Also, they are called within methods that override the base class ones. However, if it is preferable that subclasses do not contain methods that are not in the base class interface, we can either inline the specialized code and remove the methods, or make those methods private. What do you think?
        Hide
        Phil Steitz added a comment -

        Thanks for doing this, Gilles. I am fine with getting rid of the double[] clutter and also tossing one of getData() and toArray(). I don't really care which one. I guess I would marginally prefer toArray as the keeper.

        I just committed a bunch of javadoc improvements (at least I hope they are improvements) in r1162543.

        Show
        Phil Steitz added a comment - Thanks for doing this, Gilles. I am fine with getting rid of the double[] clutter and also tossing one of getData() and toArray(). I don't really care which one. I guess I would marginally prefer toArray as the keeper. I just committed a bunch of javadoc improvements (at least I hope they are improvements) in r1162543.
        Hide
        Gilles added a comment -

        Renaming done in revision 1162511.

        Show
        Gilles added a comment - Renaming done in revision 1162511.
        Hide
        Gilles added a comment -

        Also, as already noted elsewhere, the "getData()" method is redundant with "toArray()". One or the other should go.

        Show
        Gilles added a comment - Also, as already noted elsewhere, the "getData()" method is redundant with "toArray()". One or the other should go.
        Hide
        Gilles added a comment - - edited

        From Sébastien, on the "dev" ML (in the thread with subject "RealMatrix.set(double)"):

        I personnaly have come to dislike the schizophrenia in the RealVector
        interface between double[] and RealVector. As double[] is the simplest
        representation of a vector, all methods which take a RealVector as an
        argument in the RealVector interface are duplicated to also take a
        double[] as an argument. While this is very flexible for end-users, it
        is a bit of a pain when you want to extend this interface in a
        consistent way (and it also make the classes implementing RealVector
        quite cluttered). I'm just wondering what the real benefit is, since
        the existing hierarchy allows (at virtually no cost) the creation of a
        RealArrayVector from a double[] without taking first a (costly) deep
        copy of the specified double[].
        For example, for an end-user, it's not much of a hassle to write
        v.add(new ArrayRealVector(w, false))
        instead of v.add(w)
        w being a double[].

        I agree.
        Also for the reason that in "AbstractRealVector" many of the methods taking a "double[]" argument actually delegate to the sibling method that taking an "ArrayRealVector" argument, which looks fairly strange (a superclass relying on one of its subclasses). Even stranger, some methods of the "RealVector" interface are not defined in "AbstractRealVector" e.g.

        RealVector ebeDivide(RealVector)
        

        although its equivalent with a "double[]" is defined and seems to call the non-existent one above!
        Of course, it works thanks to polymorphism and the fact that "AbstractRealVector" can never be instantiated; however it makes the code quite unobvious.

        Hence, I'd like to remove all the methods that take a "double[]" argument as part of this issue.
        Is there any objection?

        Show
        Gilles added a comment - - edited From Sébastien, on the "dev" ML (in the thread with subject "RealMatrix.set(double)"): I personnaly have come to dislike the schizophrenia in the RealVector interface between double[] and RealVector. As double[] is the simplest representation of a vector, all methods which take a RealVector as an argument in the RealVector interface are duplicated to also take a double[] as an argument. While this is very flexible for end-users, it is a bit of a pain when you want to extend this interface in a consistent way (and it also make the classes implementing RealVector quite cluttered). I'm just wondering what the real benefit is, since the existing hierarchy allows (at virtually no cost) the creation of a RealArrayVector from a double[] without taking first a (costly) deep copy of the specified double[]. For example, for an end-user, it's not much of a hassle to write v.add(new ArrayRealVector(w, false)) instead of v.add(w) w being a double[]. I agree. Also for the reason that in "AbstractRealVector" many of the methods taking a "double[]" argument actually delegate to the sibling method that taking an "ArrayRealVector" argument, which looks fairly strange (a superclass relying on one of its subclasses). Even stranger, some methods of the "RealVector" interface are not defined in "AbstractRealVector" e.g. RealVector ebeDivide(RealVector) although its equivalent with a "double[]" is defined and seems to call the non-existent one above! Of course, it works thanks to polymorphism and the fact that "AbstractRealVector" can never be instantiated; however it makes the code quite unobvious. Hence, I'd like to remove all the methods that take a "double[]" argument as part of this issue. Is there any objection?

          People

          • Assignee:
            Gilles
            Reporter:
            Gilles
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development