Make some methods of Reach classes operate with arrays of objects.

[GoogleCodeExporter]

Some methods of old 'reach' class can operate on arrays of objects, but new 
'ReachContinuous' class doesn't support that approach.
Here is the list of 'reach' class methods, which can work with matrices of 
'reach' objects:
dimension()
getAbsTol()
iscut()
isprojection()
isempty()

Original issue reported on code.google.com by heartofm...@gmail.com on 8 Apr 2013 at 3:38

[GoogleCodeExporter]

Please talk to Kirill about the details. This is one week task.

Original comment by heartofm...@gmail.com on 1 May 2013 at 4:23

[GoogleCodeExporter]

Original comment by DmitryK...@gmail.com on 4 May 2013 at 10:58

• Changed state: ReadyForReview

[GoogleCodeExporter]

Please implement the tests and then I'll make a full review

Original comment by heartofm...@gmail.com on 6 May 2013 at 2:20

• Changed state: ReviewFailed

[GoogleCodeExporter]

Original comment by DmitryK...@gmail.com on 7 May 2013 at 6:06

• Changed state: ReadyForReview

[GoogleCodeExporter]

1) 

Please put the following code into checkIfNotEmpty method of AReach

+   import modgen.common.throwerror;
    38  +   if sum(size(self)<=0)
    39  +   throwerror('wrongInput:badDimensionality',...
    40  +   'each dimension of an object array should be a positive number');
    41  +   end

this method should work similarly to ellipsoid.checkIfScalar

Also, writing if sum(size(self)<=0) is a bad idea - 

because

a) sum(size(self)<=0) is not logical
b) does the same as isempty(self)

Please just write if isempty(self)

2) Using random sizes is a bad idea because the potential bugs become hard to 
reproduce. Imagine a situation when
the your test fails and you want someone to investigate the bug. You hand your 
test to other developer who laucnhes it and
discovers that it passes because randi generated different set of numbers. Thus 
you need to just to check the methods on 
a fixed set of different sizes.

3) we need repMat and getCopy for both ReachDiscrete and ReachContinuous.

Once Igor finishes his task both classes will have the same set of fields
and getCopy can be implemented in AReach but for now please add separate 
implementations
of getCopy into ReachDiscrete and ReachContinuous. repMat can already be placed 
into AReach.

Both getCopy and repMat should be declared in IReach interface.

Original comment by heartofm...@gmail.com on 8 May 2013 at 6:50

• Changed state: ReviewFailed

[GoogleCodeExporter]

4) Your code for testing iscut, isprojection, dimenstion and other methods is 
quite similar, for instance there following piece of code is just a 
copy-pasting:

    118     +   if ~isequal(dimMat, reachObjMat.dimension())
    119     +   failMsg = sprintf('failure for dimension() method; %s',failMsg);
    120     +   end
    121     +   if ~isequal(absTolMat, reachObjMat.getAbsTol())
    122     +   failMsg = sprintf('failure for getAbsTol() method; %s',failMsg);
    123     +   end
    124     +   if ~isequal(isCutMat, reachObjMat.iscut())
    125     +   failMsg = sprintf('failure for iscut() method; %s',failMsg);
    126     +   end
    127     +   if ~isequal(isProjMat, reachObjMat.isprojection())
    128     +   failMsg = sprintf('failure for isprojection() method; %s',failMsg);
    129     +   end
    130     +   if ~isequal(isEmpMat, reachObjMat.isempty())
    131     +   failMsg = sprintf('failure for isempty() method; %s',failMsg);
    132     +   end

Please create a subfunction/nested function parameterized by the method name 
and call it as many times as many methods you tests. See 
elltool.core.test.mlunit.GenEllipsoidTestCase for a demonstration of a proper 
use of nested functions and subfunctions.

Original comment by heartofm...@gmail.com on 8 May 2013 at 12:25

[GoogleCodeExporter]

5) Please remove the checks for getCopy and array methods from testRefMisc 
test. We need completely separate tests for 
a) array methods
b) repMat
c) getCopy

6) Please do not use ReachContinuous constructor directly, always use a factory 
to instantiate the reach objects.

7) All methods you add need to have proper help headers.
8) Please consider the following example:

>> res=ellipsoid.empty(0,0,1,0,1,0);
>> res.dimension

ans =

   Empty array: 0-by-0-by-1-by-0-by-1-by-0

Obviously "dimension" method of "ellipsoid" class works for empty arrays of 
ellipsoids. I think we need the same for reach classes. Please add the tests 
for that.

Original comment by heartofm...@gmail.com on 8 May 2013 at 12:33

[GoogleCodeExporter]

Original comment by DmitryK...@gmail.com on 9 May 2013 at 8:30

• Changed state: ReadyForReview

[GoogleCodeExporter]

6) Please eliminate all calls of ReachContinous by putting reachFactObj into a 
field of ContinuousReachRefineTestCase test case and calling createInstance 
method. I know that this most probably not your code but we need to do this 
anyways and since you are the only work currently working on this test case you 
are in an ideal position to do that.

8) is not fixed as your methods do not work for empty arrays. Please make sure 
that they do and add tests for that.

5) Instead of making repMat not call getCopy for empty arrays you need to make 
getCopy work even for empty arrays. 

4) The following code contains a bit of copy-pasting, please use nested 
function parameterized by a method name:

48  +   function checkArrayMethods(self, sizeVec)
    49  +   reachSingleObj=self.reachObj;
    50  +   reachObjMat = reachSingleObj.repMat(sizeVec);
    51  +   dimMat = repmat(reachSingleObj.dimension(),sizeVec);
    52  +   isCutMat = repmat(reachSingleObj.iscut(),sizeVec);
    53  +   isProjMat = repmat(reachSingleObj.isprojection(),sizeVec);
    54  +   isEmpMat = repmat(reachSingleObj.isempty(),sizeVec);
    55  +   checkEq(dimMat,reachObjMat,'dimension');
    56  +   checkEq(isCutMat,reachObjMat,'iscut');
    57  +   checkEq(isProjMat,reachObjMat,'isprojection');
    58  +   checkEq(isEmpMat,reachObjMat,'isempty');

Original comment by heartofm...@gmail.com on 11 May 2013 at 12:55

• Changed state: ReviewFailed

[GoogleCodeExporter]

9) Please remote "isNotEmpty" method - builtin isempty function works perfectly 
for objects. 
Creating a separate method for this is an overkill.

10) There is a special method in each Matlab class for creating empty arrays. 
Its name is "empty":

emptyArr=elltool.reach.ReachDiscrete.empty(0,1,0,0,1,0); 

Please replace repmat in "repmat(elltool.reach.ReachDiscrete(),size(self));" 
and similar places
with "empty".

Original comment by heartofm...@gmail.com on 12 May 2013 at 7:15

[GoogleCodeExporter]

Original comment by DmitryK...@gmail.com on 13 May 2013 at 11:45

• Changed state: ReadyForReview

[GoogleCodeExporter]

Original comment by heartofm...@gmail.com on 13 May 2013 at 7:10

• Changed state: ReadyForReintegration

[GoogleCodeExporter]

Original comment by DmitryK...@gmail.com on 13 May 2013 at 10:15

• Changed state: Reintegrated

[GoogleCodeExporter]

Original comment by heartofm...@gmail.com on 14 May 2013 at 6:15

• Changed state: Fixed