Cover all methods of reach class for discrete systems with unit tests

[GoogleCodeExporter]

You should crete elltool.reach.test.mlunit package, 
it would be located in 
<YourWorkingCopy>\products\+elltool\+reach\+test\+mlunit. 

In this package you should create ReachDiscrTestCase MLUNITEXT test class, the 
name of the file would 
be  
<YourWorkingCopy>\products\+elltool\+reach\+test\+mlunit\ReachDiscrTestCase.m

Along with this class you should 
 # create run_tests function in <YourWorkingCopy>\products\+elltool\+reach\+test folder which 
would put it into elltool.reach.test package
 # Modify elltool.test.run_tests function so that it calls 
your newly created elltool.reach.test.run_tests function in the way described 
in 
item 3 on 

http://code.google.com/p/ellipsoids/wiki/MLUNITEXT_unit_testing_framework_descri
ption 

Wiki page

Once all this is done you need to write unit tests for all the methods of 
@reach class for discrete systems.

The idea of a unit test is that you think of an analytical example where the 
output is known in advance and compare this 
expected output with the output produced by the function or/and method. 

Original issue reported on code.google.com by heartofm...@gmail.com on 3 Jan 2013 at 12:29

[GoogleCodeExporter]

Original comment by Bricker...@gmail.com on 3 Jan 2013 at 2:23

• Changed state: Started

[GoogleCodeExporter]

Original comment by Bricker...@gmail.com on 15 Apr 2013 at 11:33

• Changed state: ReadyForReview

[GoogleCodeExporter]

1) Answering your question about minksum_ea - pelase replace sqrt with realsqrt 
in ellipsoid.minksum_ea and use runAndCheckError method of test_case to 
implement a negative check in testOverflow test (see 
http://code.google.com/p/ellipsoids/wiki/MLUNITEXT_unit_testing_framework_descri
ption for more information)

3) Regarding the following example
AMat = [3 0 2; 2 2 2; -1 0 3];
            BMat = [0 1 0; 0 1 0; 3 2 1];
            PEll = ellipsoid([3 2 1]', [4 2 0; 2 4 0; 0 0 2]);
            BMat * [4 2 0; 2 4 0; 0 0 2] * BMat'
            LS = elltool.linsys.LinSysFactory.create(AMat, BMat, PEll, [], [], [], [], 'd');
            X0Ell = ellipsoid([-1 -2 1]', diag([3, 2, 1]));
            LMat = eye(3);
            RS = elltool.reach.ReachDiscrete(LS, X0Ell, LMat, [1, 20]);

Firsty Please an obligatory input parameter absTol to ell_regularize so that it 
is passed from outside as opposed to using Properties.getAbsTol for its 
extraction.
Then, please note that in ell_regularize U * V' is expected to be identity 
matrix but the authors of the function forgot that the vectors from
 U and V are parallel but not collinear which may result in -1 on the diagonal of U*V.'; This please use eig instead of svd:

[uMat,¬]=eig(origMat);
origMat=origMat+delta*uMat*UMat;

And please add a test for elltool.core.test.mlunit.EllAuxTestCase to make sure 
that ell_regularize works correctly after your fix

4) An amount of copy-pasting in your tests is insane. You need to fix this by 
using both subfunctions and nested functions (see 
http://www.mathworks.co.uk/help/matlab/matlab_prog/nested-functions.html) 
Please note that the nested functions inherit a context which is extremely 
useful when writing the tests (i.e nested functions are closures implementation 
in Matlab - see http://en.wikipedia.org/wiki/Closure_%28computer_science%29). 
Please refer to elltool.core.test.mlunit.GenEllipsoidTestCase test case as an 
ethalon of test case implementation as an example of both subfunctions and 
closures use.

5) I could hardly find a variable which was named correctly in your test case - 
please study the table from 
http://code.google.com/p/ellipsoids/wiki/Coding_policy very carefull to see 
what is allow and what is not.

First of all, variable names should always start with a small letter unless it 
is a structure. I'm aware that we still have some legacy code that doesn't 
comply with this convention but we will have it fixed overtime. New code should 
comply with the convention 100%. And please do not forget about the suffixes 
(Mat for matrices, Vec for vecors, Arr for 3>= dimensional arrays).

6) Your code is not vectorized, everything you do can be done without any 
loops. Please get rid of the loops (I'll provide a few hints how this can be 
done in the comments to your last commit).

Original comment by heartofm...@gmail.com on 16 Apr 2013 at 2:31

• Changed state: ReviewFailed

[GoogleCodeExporter]

7) Please keep all lines under 76 symbols, use "..." to make them shorter.

8) Also, please use OOP convention for calling the methods:

myMethod(myObj,param1,param2) is bad

myObj.myMethod(param1,param2) is good, with new matlab it is possible

9) Avoid using ^2, ^0.5 for scalars and matrices, use
gras.la.sqrtmpos 
realsqrt
x*x
instead

which is more precise and faster

Original comment by heartofm...@gmail.com on 16 Apr 2013 at 2:55

[GoogleCodeExporter]

>Firsty Please an obligatory input parameter absTol to ell_regularize so that 
it is >passed from outside as opposed to using Properties.getAbsTol for its 
extraction.

Looks like you forgot a predicate. I didn't understand, what do you mean.

>origMat=origMat+delta*uMat*UMat;

upper - lower case and ' mismatch. i hope i've made what you mean.

Original comment by Bricker...@gmail.com on 19 Apr 2013 at 1:55

• Changed state: ReadyForReview

[GoogleCodeExporter]

1) I meant that you need to pass absTol as an input parameter (in 
ell_regularize func) as opposed to using a value extracted from 
elltool.conf.Properties.getAbsTol

2) Let's hope so

Original comment by heartofm...@gmail.com on 19 Apr 2013 at 9:35

[GoogleCodeExporter]

0) Commented out code - you should not leave it.
1) Test for ell_regularize should be added into 

  elltool.core.test.mlunit.EllAuxTestCase

test case (this test case needs to be created)

The test doesn't test anything

2) createReach subfunction is not used everywhere
3) variable names are still off (no suffixes, capital letters etc)

4)  You can use arrayfun to getrid of both loops here:

                    for iDirection = 1 : 3

                        for t = t1 : t2
                            [q1 Q1] = expectedApproxEllMat(iDirection, t).double();
                            [q2 Q2] = obtainedApproxEllMat(iDirection, ...
                                t - t1 + 1).double();
                            isOk = isOk && max(abs(q1 - q2)) < epsilon;
                            resMat = abs(Q1 - Q2);
                            isOk = isOk && max(resMat(:)) < epsilon;
                        end
                    end

5) rho method is not in all the places

6) In the loop below 

a) use \ instead of inv (inv is slow)
b) 

                for jDirection = 1 : 3
                    dirMat(:, 1) = lMat(:, jDirection);
                    if (~iscell(aMat))
                        for iTime = 2 : T
                            dirMat(:, iTime) = (aMat')^(-1) *...
                                dirMat(:, iTime - 1);
                        end
                    else
                        k = 1;
                        curAMat = cellfun(@eval, aMat);
                        for k = 2 : T
                            dirMat(:, k) = inv(curAMat') * dirMat(:, k - 1);
                            curAMat = cellfun(@eval, aMat);
                        end
                    end
                    expectedDirectionsCVec{jDirection} = dirMat;
                end
7) Fix all mlink warnings (see 
http://www.mathworks.com/help/matlab/matlab_prog/check-code-for-errors-and-warni
ngs.html#brqxeeu-156)

Original comment by heartofm...@gmail.com on 19 Apr 2013 at 9:51

• Changed state: ReviewFailed

[GoogleCodeExporter]

>I meant that you need to pass absTol as an input parameter (in ell_regularize 
func) as opposed to using a value extracted from 
elltool.conf.Properties.getAbsTol

It also passes as input parameter. If it doesn't input, function takes 
Properties.getAbsTol value.

Original comment by Bricker...@gmail.com on 21 Apr 2013 at 3:14

[GoogleCodeExporter]

You need to make this input parameter obligatory so that absTol is always 
passed from outside. This way we can guarantee that we use a correct value of 
absTol.

Original comment by heartofm...@gmail.com on 21 Apr 2013 at 3:24

[GoogleCodeExporter]

Please summarize all i more have to implement, fix and debug in one list.

Original comment by Bricker...@gmail.com on 21 Apr 2013 at 4:08

• Changed state: ReadyForReview

[GoogleCodeExporter]

I've already described all the problems, if you have them fixed - please let me 
know. I do not have time to to re-type what I already typed just to save your 
time.

Also, the test in the trunk fails for ReachDiscrete - please reproduce this 
problem by one of your tests and make it fixed.

Original comment by heartofm...@gmail.com on 21 Apr 2013 at 7:11

• Changed state: ReviewFailed

[GoogleCodeExporter]

Yes, i have fixed what you described. Review, please.

Original comment by Bricker...@gmail.com on 21 Apr 2013 at 9:01

[GoogleCodeExporter]

I meant summarize the rest.

Original comment by Bricker...@gmail.com on 21 Apr 2013 at 9:05

[GoogleCodeExporter]

I'll review tomorrow in the evening. If you have time until then - please deal 
with the bug in ReachDiscrete revealed by 
elltool.reach.test.mlunit.DiscreteReachTestCase('testFirstBasicTest') test. 
Since this is the only bug for ReachDiscrete that I know of once it is fixed 
you are done.

Original comment by heartofm...@gmail.com on 21 Apr 2013 at 9:09

[GoogleCodeExporter]

The point of error contains in strings 91 and 123 of minksum_ia.m, and at 
string 128 error occures. Same tolerance in sqrtmpos and ellipsoid.regularize 
leads to creation of degenerated ellipsoids after sqrtmpos. If we make 
tolerance at sqrtmpos smaller, we can avoid this error.

Original comment by Bricker...@gmail.com on 21 Apr 2013 at 11:17

• Changed state: ReadyForReview

[GoogleCodeExporter]

1) test in EllAuxTestCase doesn't check ell_regularize. No need to test 
gras.la.ismatposdef - it is already covered by tests.

2) This code can be made almost 2 times shorter:

                sourceEaEllMat = rs.get_ea();
                sourceIaEllMat = rs.get_ia();
                if (numel(cutTVec) == 1)
                    rs2 = rs.cut(cutTVec);
                    cutEaEll = rs2.get_ea();
                    cutIaEll = rs2.get_ia();
                    isOk = isOk && all(sourceEaEllMat(:, cutTVec) ==...
                        cutEaEll(:));
                    isOk = isOk && all(sourceIaEllMat(:, cutTVec) ==...
                        cutIaEll(:));
                else
                    rs2 = cut(rs, cutTVec);
                    [cutEaEllMat tVec] = rs2.get_ea();
                    isResultMat = sourceEaEllMat(:, cutTVec(1):cutTVec(2))...
                        == cutEaEllMat;
                    isOk = isOk && all(isResultMat(:));
                    isOk = isOk && all(tVec == cutTVec(1):cutTVec(2));
                    [cutIaEllMat tVec] = rs2.get_ia();
                    isResultMat = sourceIaEllMat(:, cutTVec(1):cutTVec(2)) ...
                        == cutIaEllMat;
                    isOk = isOk && all(isResultMat(:));          
                    isOk = isOk && all(tVec == cutTVec(1):cutTVec(2));

                end

just create a nested function parameterized by rs2 method handle (either get_ia 
or get_ea

Same problem is in testProjection test - the code for internal approximation 
and external approximation is the same. You also need to create a nested 
function.

Same in testEvolve test in auxTestEvolve nested function.

Original comment by heartofm...@gmail.com on 22 Apr 2013 at 10:38

• Changed state: ReviewFailed

[GoogleCodeExporter]

>1) test in EllAuxTestCase doesn't check ell_regularize. No need to test 
gras.la.ismatposdef - it is already covered by tests.

It test, i just have forgot one string.

I can't run all amount of tests, because i have memory limit errors on my 
notebook.

Original comment by Bricker...@gmail.com on 22 Apr 2013 at 8:30

• Changed state: ReadyForReview

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Original comment by heartofm...@gmail.com on 22 Apr 2013 at 9:15

• Changed state: ReadyForReview

[GoogleCodeExporter]

The only thing remaining is moving testSqrtmposToleranceFailure test into 
elltool.core.test.mlunit.EllipsoidTestCase.

Original comment by heartofm...@gmail.com on 22 Apr 2013 at 9:21

• Changed state: ReviewFailed

[GoogleCodeExporter]

The thing #2 is that there is one failed test in another test pack (see below). 
I expect that you might say that this is not your test but the problem is that 
you are expected to fix the bugs in ReachDiscrete and this test seem to 
discover one by  comparing internal and external approximations by putting all 
the tubes into gras.ellapx.smartdb.rels.EllTube container. This container 
performs the consistency checks like:

a) All internal approximations lay within every external approximation
b) Touch curves (support vectors x^{*}(t) corresponding to good directions 
l(t)) do not depend on the approximation type (i.e. if some approximation is 
tight along l(t), it should matter whether x^{*}(t) is calculated from external 
approximation or from internal approximation).

And the test failure show below seems to indicate that the check (b) doesn't 
pass. Please have it investigated. If you prove that the test is incorrectly 
written - ok, fine. But if there is a bug - I have to ask you to fix it.

 INFO mlunit.logprintf - ======================================================================
 INFO mlunit.logprintf - ERROR: elltool.reach.test.mlunit.DiscreteReachTestCase('testConsistency')
 INFO mlunit.logprintf - ----------------------------------------------------------------------
 INFO mlunit.logprintf - Traceback (most recent call first): 
in 
D:\SVN_Local\EllToolBoxTrunk\products\+gras\+ellapx\+smartdb\+rels\EllTubeTouchC
urveBasic.m at line 172
in 
D:\SVN_Local\EllToolBoxTrunk\products\+gras\+ellapx\+smartdb\+rels\EllTubeTouchC
urveBasic.m at line 158
in 
D:\SVN_Local\EllToolBoxTrunk\products\+gras\+ellapx\+smartdb\+rels\EllTubeTouchC
urveBasic.m at line 113
in 
D:\SVN_Local\EllToolBoxTrunk\products\+gras\+ellapx\+smartdb\+rels\EllTubeBasic.
m at line 302
in D:\SVN_Local\EllToolBoxTrunk\products\+gras\+ellapx\+smartdb\+rels\EllTube.m 
at line 7
in 
D:\SVN_Local\EllToolBoxTrunk\lib\+smartdb\+cubes\@CubeStruct\addDataAlongDimInte
rnal.m at line 232
in 
D:\SVN_Local\EllToolBoxTrunk\lib\+smartdb\+cubes\@CubeStruct\unionWithAlongDimIn
ternal.m at line 80
in D:\SVN_Local\EllToolBoxTrunk\lib\+smartdb\+relations\@ARelation\unionWith.m 
at line 39
in 
D:\SVN_Local\EllToolBoxTrunk\products\+elltool\+reach\+test\+mlunit\DiscreteReac
hTestCase.m at line 288
in 
D:\SVN_Local\EllToolBoxTrunk\products\+elltool\+reach\+test\+mlunit\DiscreteReac
hTestCase.m at line 281
in D:\SVN_Local\EllToolBoxTrunk\lib\mlunitext\+mlunit\test_case.m at line 141
in D:\SVN_Local\EllToolBoxTrunk\lib\mlunitext\+mlunitext\test_case.m at line 88
in D:\SVN_Local\EllToolBoxTrunk\lib\mlunitext\+mlunit\test_suite.m at line 289
in D:\SVN_Local\EllToolBoxTrunk\lib\+modgen\+pcalc\auxdfeval.m at line 223
in D:\SVN_Local\EllToolBoxTrunk\lib\mlunitext\+mlunitext\+pcalc\auxdfeval.m at 
line 12
in D:\SVN_Local\EllToolBoxTrunk\lib\mlunitext\+mlunitext\test_suite.m at line 
191
in D:\SVN_Local\EllToolBoxTrunk\lib\mlunitext\+mlunit\text_test_runner.m at 
line 73
in 
D:\SVN_Local\EllToolBoxTrunk\products\+elltool\+reach\+test\run_discrete_reach_t
ests.m at line 29

Error: value of field xTouchCurveMat is expected to be dependent only on 
(sTime,lsGoodDirVec,MArray),actual tolerance=71.4351, expected tolerance=0.002, 
Identifier: 
GRAS:ELLAPX:SMARTDB:RELS:ELLTUBETOUCHCURVEBASIC:CHECKTOUCHCURVEINDEPENDENCE:wron
gInput:touchCurveDependency

Original comment by heartofm...@gmail.com on 22 Apr 2013 at 9:42

[GoogleCodeExporter]

The test fails with next data:
one set of values of xTouchCurveMat is

   1.0e+06 *

  Columns 1 through 3

                   0  -0.000011000055000   0.000017701513508
   0.000000100000500  -0.000000800004000  -0.000016085151675

  Columns 4 through 6

   0.000151453535528  -0.001638586605939   0.010152050634445
   0.000164767729962  -0.001016068082229   0.004852381809408

  Columns 7 through 9

  -0.048516501977894   0.185168669805005  -0.511001955850604
  -0.018516316664387   0.051099199284065  -0.038456082681977

  Columns 10 through 11

   0.384570455924035   7.143548566287447
  -0.714355789924966   6.483987949490543

and delta with other values is 

Columns 1 through 3

                   0  -0.000110000000001   0.000177014250013
   0.000001000000000  -0.000008000000000  -0.000160850712501

  Columns 4 through 6

   0.001514527782831  -0.016385784128033   0.101519998746880
   0.001647669061413  -0.010160630018845   0.048523575480431

  Columns 7 through 9

  -0.485162593999121   1.851677440106869  -5.109994011058006
  -0.185162240875798   0.510989438138495  -0.384558905177983

  Columns 10 through 11

   3.845685343083460  71.435128473676741
  -7.143522177706473  64.839555296115577

It looks like relative tolerance is about 1e-5. I don't think there is some 
logic error, just cumulative computation error, considering that there used in 
toolbox some kinds of regularization in many places.

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 1:16

• Changed state: ReadyForReview

[GoogleCodeExporter]

It is strangely to expect low absolute tolerance on such big values. I think 
would be better to control relative tolerance.

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 1:20

[GoogleCodeExporter]

OK, please 

1) Disable testConsistency test(rename it to DISABLED_testConsistency)

2) 
uncomment continuous test runs in

    Modify  /branches/issue_58_akuznetzov/products/+elltool/+reach/+test/run_discrete_reach_tests.m diff

    Modify  /branches/issue_58_akuznetzov/products/+elltool/+reach/+test/run_tests.m    diff

    Modify  /branches/issue_58_akuznetzov/products/+elltool/+test/run_tests.m

and then reintegrate. 

I'll wait for the tests to pass in trunk. If everything is ok - you are done. 

Original comment by heartofm...@gmail.com on 23 Apr 2013 at 7:23

• Changed state: ReadyForReintegration

[GoogleCodeExporter]

When reintegrating please follow the algorithm described at 
http://code.google.com/p/ellipsoids/wiki/Issues_workflow_and_branching_policy

Original comment by heartofm...@gmail.com on 23 Apr 2013 at 11:40

[GoogleCodeExporter]

As i already said, i can't run elltool.test.run_tests on my workstation. Should 
i commit without passing tests after step 6?

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 3:08

[GoogleCodeExporter]

Try to run only elltool.reach.test.run_tests. Before running the tests please 
delete all subfolders but "_templates" from 
\products\+gras\+ellapx\+uncertcalc\+test\+regr\+conf\confrepo

Original comment by heartofm...@gmail.com on 23 Apr 2013 at 3:14

[GoogleCodeExporter]

elltool.reach.test.run_test passed with no errors and failures.
There is no subfolders in 
\products\+gras\+ellapx\+uncertcalc\+test\+regr\+conf\confrepo bu _templates.
Commit?

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 4:29

[GoogleCodeExporter]

That is strange because elltool.reach.test.run_test also starts the 
ReachContinuous tests as well. These tests create a subfolder in 
\products\+gras\+ellapx\+uncertcalc\+test\+regr\+conf\confrepo with a name 
corresponding to the name of your computer. 

Can it be that you only ran the discrete tests?

Original comment by heartofm...@gmail.com on 23 Apr 2013 at 4:37

[GoogleCodeExporter]

Yes, after execution of tests in this folder there is a folder with my IP. 
Before execution there was only _tepmlates. I have never launched tests in 
trunk.

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 4:43

[GoogleCodeExporter]

I am sure that i launched both discrete and continious tests.

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 4:46

[GoogleCodeExporter]

Should i commit?

Original comment by Bricker...@gmail.com on 23 Apr 2013 at 5:17

[GoogleCodeExporter]

Yes.

Original comment by heartofm...@gmail.com on 23 Apr 2013 at 5:49

[GoogleCodeExporter]

Original comment by heartofm...@gmail.com on 24 Apr 2013 at 1:00

• Changed state: Fixed