Cover problem dynamics and problem definition classes with tests

[pgagarinov]

Cover problem dynamics and problem definition classes from the following packages with tests \products+gras+ellapx+lreachplain+probdef \products+gras+ellapx+lreachplain+probdyn \products+gras+ellapx+lreachuncert+probdef \products+gras+ellapx+lreachuncert+probdyn

the test cases should be located in the following packages

\products+gras+ellapx+lreachplain+probdef+test+mlunit \products+gras+ellapx+lreachplain+probdyn+test+mlunit \products+gras+ellapx+lreachuncert+probdef+test+mlunit \products+gras+ellapx+lreachuncert+probdyn+test+mlunit

functions run_tests should be located in each +test subpackage. and their calls should be added to \products+gras+ellapx+test\run_tests function.

For the problem definition classes (in probdef packages) the tests should construct the problem definition objects on a few simple systems and verify that all methods return a correct result. Please note that there should be no copy-pasting in the tests - you should parameterize them with tested calsses so that the same test case can be applied to different problem definition classes. This approach is very well demonstrated in elltool.reach.test.run_cont_tests - please use this function as a reference.

For problem dynamics classes the approach should be similar - create problem dynamics for a few simple systems and make sure that the values returned by its methods (by getAtDynamics method responsible for returning a matrix function for A(t)) match the expected result on a certain time grid. For A(t) it means that you need to check that A(t_i) matches the expected result for all t_i from the time grid.

[arturlu]

@pgagarinov I have added simple tests for problem definition. Can you look on it?

There are some issues of this code:

• code duplication in run_tests(). I can merge generalize these functions. But where I must place generalized code? into gras.ellapx.test.run_test() ?
• Ugly long string into +lreachuncert+probdef+test+mlunit\ProbDefUncertTC.m self@gras.ellapx.lreachplain.probdef.test.mlunit.ProbDefPlainTC. Is it ok? (import doesn't work).
• current testGetters() method doesn't compare the result with input. Is there any sense to add these trivial comparisons?

[pgagarinov]

1) suites = {}; -> suiteList = {}; (naming convention!)

2) In resList = runner.run(suite); results=[resList]; please a) omit [] b) fix resList name - see naming convention

3) fConstrCVec is a bad name as f prefix is only for function handles (see naming convention). confNamesCVec is also a bad name.

4)

code duplication in run_tests(). I can merge generalize these functions. But where I must place >generalized code? into gras.ellapx.test.run_test() ?

As far as I can tell the only difference is a package name, if so - please create I suggest you create products\+gras\+ellapx\+lreachplain\+probdef\+test\run_tests_for_package parameterized by a package name. You can then run this function from both run_tests functions.

5)

current testGetters() method doesn't compare the result with input.

First of all the checks are not that trivial for dynamics classes where the getters return objects. And secondly, yes, writing thorough not copy-pasted checks is absolutely necessary. Here is why. We both know that current classes work and most likely there are no any bugs thought there is no guarantee that there is no any. However we need the tests not just to squash unlikely bugs in the existing code - we need them

• to fix current functionality so that when developers start changing code by adding new features they do not break existing functionality
• as a documentation for developers because tests are very good for studying how classes work, especially for a developer. Documentation (help headers) sometimes is not ideal because it is much more difficult to maintain and keep up to date (you change implementation and forget to update the docs and voila!). Tests are always up-to-date, otherwise they just do not pass.

[arturlu]

@pgagarinov I have updated tests. Did I make a bicycle when have implemented cellcmp function?

As for dynamics classes. Dynamics classes are additionally divided on to "constant" *LTIDynamics classes and time depended *DynamicsInterp classes. Do I correctly understand that I can use symbolic functionality from gras.mat.MatrixOperationsFactory package to compute expected values in tests (for both "constant" and "time depended" classes)?

[pgagarinov]

I do not see any new commits. Yes, you can use this class.

[pgagarinov]

Good job indeed, this is exactly what we needed. There is a few things to fix though:

6) assert_cell_equals->assertCellEquals and yes, compcell is not necessary you can just write mlunitext.assert(isequal(aCellVec,bCellVec)); 7) At->bigAtObj. 8) Use isParam method and never use try-catch for normal code execution. try self.cCMat = crmSys.getParam('Ct'); self.qCMat = crmSys.getParam('disturbance_restriction.Q'); self.qCVec = crmSys.getParam('disturbance_restriction.a'); self.isUncert = true; catch self.isUncert = false; end 9) package is extremely bad name -see our naming convention

10) XtFunc - bad, see our naming convention. There is special prefix for function handles.

11) I suspect that the following properties can be either private or at least protected:

properties
    aCMat
    bCMat
    cCMat
    pCMat
    pCVec
    qCMat
    qCVec
    x0Mat
    x0Vec
    tLims

    isUncert
end

12) The following code can be vectorized as "evaluate" can accept vectors:

        for iElem=1:numel(self.tVec)-1
            t0 = self.tVec(iElem);
            t1 = self.tVec(iElem+1);
            t = 0.5*(t0+t1);

            xVec = XtFunc(t);
            dxVec = (XtFunc(t1) - XtFunc(t0)) / (t1 - t0);
            dxRefVec = XtDerivFunc(t, xVec);

            [isEqual,absDif,~,relDif] = absrelcompare(dxRefVec,dxVec,...
                TOL_MULT*self.absTol, TOL_MULT*self.relTol, @norm);

            mlunitext.assert(isEqual,...
                sprintf(['xtDynamics check failed at '...
                't=%f: absDif=%f, relDif=%f'], t0, absDif, relDif));
        end

       for iElem=1:numel(self.tVec)
           t = self.tVec(iElem);
           et1Mat = mat1Obj.evaluate(t);
           et2Mat = mat1Obj.evaluate(t);
           isEqual = absrelcompare(et1Mat, et2Mat,...
               self.absTol, self.relTol, @norm);

           mlunitext.assert(isEqual,...
               sprintf(['%s matrix function check failed '...
               'at t=%f'], inputname(2), t));
       end

13) REL_ABS_TOL = {1e-6, 1e-8}; - bad name, no cell-specific suffix

14) In

suiteDefList = { struct(... 'defConstr', @gras.ellapx.lreachplain.probdef.LReachContProblemDef,... 'dynConstr', @gras.ellapx.lreachplain.probdyn.LReachDiscrForwardDynamics,... 'confs', {confList},... 'TC', 'gras.ellapx.lreachplain.probdyn.test.mlunit.ProbDynPlainDiscrTC'... ); };

structure names are not consistent with our naming convention

15) Please remove commented code such as the following everywhere:

import gras.ellapx.lreachplain.probdyn.test.run_discr_tests_from_suitedef; results=run_discr_tests_from_suitedef(suiteDefList); %results=[]; end

[arturlu]

@pgagarinov Updated. Except the following:

11) I suspect that the following properties can be either private or at least protected:

This class is used by test classes as plain struct (via self.readObj member) therefore these properties cannot be protected. The constructor is needed therefore it is class and not struct.

12) The following code can be vectorized as "evaluate" can accept vectors:

Yes, XtFunc can accepts time vectors and return 3-dim array, but XtDerivFunc cannot. I vectorized the code of the check:

        t0Vec = (tVec(2:end)+tVec(1:end-1))*0.5;
        dxArray = diff(fXtFunc(tVec), 1, 3);
        dtVec   = reshape(diff(tVec), 1, 1, []);
        dtArray = repmat(dtVec,[size(dxArray,1),size(dxArray,2),1]);
        dxArray = dxArray./dtArray;
        dxRefCVec = arrayfun(fXtDeriv, t0Vec, 'UniformOutput', false); 
        dxRefMat = cat(3, dxRefCVec{:});

        [isEqual,absDif,~,relDif] = absrelcompare(...
            dxRefMat(:),dxArray(:),...
            TOL_MULT*self.absTol, TOL_MULT*self.relTol, @norm);

        mlunitext.assert(isEqual,...
            sprintf(['xtDynamics check failed:'...
            'absDif=%f, relDif=%f'], absDif, relDif));

It looks less informative than the following:

        for iElem=1:numel(tVec)-1
            t0 = tVec(iElem);
            t1 = tVec(iElem+1);
            t = 0.5*(t0+t1);

            dxVec = (fXtFunc(t1) - fXtFunc(t0)) / (t1 - t0);
            dxRefVec = fXtDeriv(t);

            [isEqual,absDif,~,relDif] = absrelcompare(dxRefVec,dxVec,...
                TOL_MULT*self.absTol, TOL_MULT*self.relTol, @norm);

            mlunitext.assert(isEqual,...
                sprintf(['xtDynamics check failed at '...
                't=%f: absDif=%f, relDif=%f'], t0, absDif, relDif));
        end

[pgagarinov]

It looks much better now but there are still a few problems:

11) If you just get the properties then you can use (SetAccess=private) which will not limit a get-access in any way.

16) Bad input names in assertCellEquals(c1, c2, msg) - see our naming convention ) This looks like a misprint: mlunitext.assert(isequal(c1, c2, msg)); as isequal accepts only 2 input arguments.

17) Still bad names and loops:

    function checkDifFun(self, XtDifFunc, XtFunc)
        import modgen.common.absrelcompare;
        TOL_MULT = 10e1;

        tVec = self.pDynObj.getTimeVec();
        for iTime=1:numel(tVec)-1
            t0 = tVec(iTime);
            t1 = tVec(iTime+1);

            dxVec = XtFunc(t1) - XtFunc(t0);
            dxRefVec = XtDifFunc(t0);

            [isEqual,absDif,~,relDif] = absrelcompare(dxRefVec,dxVec,...
                TOL_MULT*self.absTol, TOL_MULT*self.relTol, @norm);

            mlunitext.assert(isEqual,...
                sprintf(['xtDynamics check failed at '...
                't=%f: absDif=%f, relDif=%f'], t0, absDif, relDif));
        end
    end

    function checkMatFun(self, mat1Obj, mat2Obj)
       import modgen.common.absrelcompare;
       mlunitext.assert_equals(mat1Obj.getMatrixSize(),...
           mat2Obj.getMatrixSize());

       for iElem=1:numel(self.tVec)
           t = self.tVec(iElem);
           et1Mat = mat1Obj.evaluate(t);
           et2Mat = mat1Obj.evaluate(t);
           isEqual = absrelcompare(et1Mat, et2Mat,...
               self.absTol, self.relTol, @norm);

           mlunitext.assert(isEqual,...
               sprintf(['%s matrix function check failed '...
               'at t=%f'], inputname(2), t));
       end
    end

You need to fix bad names in all the code, not just where I find them.

18) Not sure why you need

methods(Static)
    function multiplyMatrixSamples(m1Array, m2Array)
        m1Array = reshape(shiftdim(m1Array, 2), [], size(m1Array, 2));
        m2Array = reshape(shiftdim(m2Array, 2), [], size(m2Array, 2));
    end
end

it doesn't seem to be used anywhere

19) Structure names in

suiteDefList = { struct(... 'defConstr', @gras.ellapx.lreachplain.probdef.LReachContProblemDef,... 'dynConstr', @gras.ellapx.lreachplain.probdyn.LReachDiscrForwardDynamics,... 'testCase', 'gras.ellapx.lreachplain.probdyn.test.mlunit.ProbDynPlainDiscrTC',... 'confs', {confList}... );

are still not consistent with our naming policy (special prefix for function handles, suffices for cells etc)

[arturlu]

@pgagarinov Fixed

[pgagarinov]

20) in

        cCMat = self.readObj.cCMat;
        CqtMatFun = matOpFactory.rSymbMultiplyByVec(...
            cCMat, self.readObj.qCVec);
        self.checkMatFun(CqtMatFun, self.pDynObj.getCqtDynamics());

        CQCTransMatFun = matOpFactory.rSymbMultiply(...
            cCMat, self.readObj.qCMat, cCMat');
        self.checkMatFun(CQCTransMatFun,...
            self.pDynObj.getCQCTransDynamics());

please use bigCMat instead of cCMat and bigCQCTrasMatFunc instead of CQCTrasMatFunc to comply with our convention

Same goes for this code:

        AtMatFun = matOpFactory.fromSymbMatrix(self.readObj.aCMat);
        self.checkMatFun(AtMatFun, self.pDynObj.getAtDynamics());

        BPBTransMatFun = matOpFactory.rSymbMultiply(...
            self.readObj.bCMat,...
            self.readObj.pCMat,...
            self.readObj.bCMat');
        self.checkMatFun(BPBTransMatFun,...
            self.pDynObj.getBPBTransDynamics());

        BptMatFun = matOpFactory.rSymbMultiply(...
            self.readObj.bCMat,...
            self.readObj.pCVec);
        self.checkMatFun(BptMatFun, self.pDynObj.getBptDynamics());

Once you fix this you can merge to master. Just be very careful and wait for the tests to pass in your branch after rebase.

[pgagarinov]

When merging to master please keep in mind that Ruslan has just merged to master. You need to wait for the tests in updated master to pass before rebasing against updated master. If tests in master fail right now - I'll have to revert Ruslan's changes. Let us wait and see.

[pgagarinov]

There are two failures in master, please wait until I fix them by a direct commit to master.

[arturlu]

Ok. I rebased to master (by using ebb71ad028840d8c1d813818f353c577049711c4 commit) and merged.

But only the two errors into: elltool.reach.test.mlunit.ContinuousReachTestNTimeGridPoints[demo3thirdTest_IsBackTrueIsEvolveFalse]/testSetMatchesGetNTimeGridPoints elltool.reach.test.mlunit.ContinuousReachTestNTimeGridPoints[demo3thirdTest_IsBackTrueIsEvolveTrue]/testSetMatchesGetNTimeGridPoints remain.