search

xjiang4 / ellipsoids

Automatically exported from code.google.com/p/ellipsoids
Other
0 stars 1 forks source link

Improve display methods for both ellipsoid and hyperplane classes #66

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
1) Make strucdisp function support 

a) Structure arrays, not just structure vectors. Right now 

strucdisp(struct('a',num2cell(zeros(2,3,4,5)),'b',num2cell(zeros(2,3,4,5)))) 
doesn't work

b) Logical fields

The tests for this function are located in 
modgen.struct.test.mlunit_test_mixed

2) Implement toStruct method in both ellipsoid and hyperplane classes. The 
methods should return a structure array like on line 44 in eq method of 
ellipsoid. This method should be covered with the unit tests.

3) Make display and eq methods of both hyperplane and ellipsoid classes use 
toStruct method

Original issue reported on code.google.com by heartofm...@gmail.com on 24 Feb 2013 at 7:56

GoogleCodeExporter commented 8 years ago

Original comment by heartofm...@gmail.com on 26 Mar 2013 at 9:27

GoogleCodeExporter commented 8 years ago 
This ticket should fix the following bug but still, better to post it here:

When you call
ell(27) = ellipsoid();
ell2 = reshape(ell,[3,3,3]);
display(ell2);
the result will be "3x9 array of ellipsoids", while size(ell2) is [3,3,3].

Original comment by heartofm...@gmail.com on 15 Apr 2013 at 5:18

GoogleCodeExporter commented 8 years ago 
[deleted comment]
GoogleCodeExporter commented 8 years ago 
[deleted comment]
GoogleCodeExporter commented 8 years ago 
[deleted comment]
GoogleCodeExporter commented 8 years ago 
[deleted comment]
GoogleCodeExporter commented 8 years ago 
1.Logical fields are already supported by strucdisp.

2. How toStruct should be used in hyperplane.display()? It should be like i 
coded?

Original comment by Alexande...@gmail.com on 30 Apr 2013 at 3:18

GoogleCodeExporter commented 8 years ago 

1) The support is not complete, see the difference:

>> strucdisp(struct('a',true(1,10)))
   |    
   |-- a : [1x10 Logic array]
>> strucdisp(struct('a',ones(1,10)))
   |    
   |-- a : [1 1 1 1 1 1 1 1 1 1]
>> 

I want to see a content of logical arrays, not just their dimensionality. 

2) Your implementation of display is absolutely incorrect. You should use 
strucdisp. 

3)
All new functionality in the methods you changed should be covered with tests. 
We do the test-driven development which assumes that before implementing a new 
functionality or fixing a bug you write a test that doesn't pass. Then you do 
the implementation/fix the bug and make sure that the tests pass.

4) Your implementation of toStruct() is incorrect, you need to create a 
structural array, not a single structure with vectorial fields.

5) By changing eq you broken because of 4) but you do not seem to notice that 
because you do not run the tests. Run elltool.core.test.run_tests and you will 
see what I'm talking about. Please run the tests for all the 
methods/functionality you change before asking me to review it. Otherwise it 
just doensn't make any sense.

6) All the methods/functions you create should have the help headers. See wiki 
for a correct help header format. The methods/functions you significantly 
change should have the copyright statement complemented with your name/email 
address. Also see Wiki for a correct copyright statement format.

Original comment by heartofm...@gmail.com on 30 Apr 2013 at 12:17

GoogleCodeExporter commented 8 years ago 
[deleted comment]
GoogleCodeExporter commented 8 years ago 
Doesn't hyperplane.display() use strucdisp?

Original comment by Alexande...@gmail.com on 5 May 2013 at 7:32

GoogleCodeExporter commented 8 years ago 
3. I prefer to ask, right way i act or not, before to write tests on my work.

4. I think i create structural array. Do not i?
A = [ell, ell];
A.toStruct() - returns 1x2 struct array.

5. I don't see, what have i broken. I ran core tests and have no errors and 
failures. All amount of tests i can't run on my machine. What set of tests 
should i look?

Original comment by Alexande...@gmail.com on 5 May 2013 at 8:04

GoogleCodeExporter commented 8 years ago 
How to understand, that my changes are enough significant?

Original comment by Alexande...@gmail.com on 5 May 2013 at 8:31

GoogleCodeExporter commented 8 years ago 
>When you call
>ell(27) = ellipsoid();
>ell2 = reshape(ell,[3,3,3]);
>display(ell2);
>the result will be "3x9 array of ellipsoids", while size(ell2) is [3,3,3].

It conflicts with way to display structural arrays in strucdisp. Strucdisp 
assumes to display first n (10) ellipsoids and then display the rest count of 
undisplayed.

Original comment by Alexande...@gmail.com on 5 May 2013 at 9:58

GoogleCodeExporter commented 8 years ago 
1) Use inputname instead of "Structure" in

Structure(1)
   |    
   |-- q : [0 0]
   |       -----
   |-- Q : |0|0|
   |       |0|0|
   |       -----
   O

Structure(2)
   |    
   |-- q : [0 0]
   |       -----
   |-- Q : |0|0|
   |       |0|0|
   |       -----
   O

If inputname returns an emptry string - use 'ellArr" as as default. "ellArr"
should be defined as a constant.

1) strucdisp needs to be used even when numel(ellArr)=1. 

Also, please use the following format (btw - please rename q into a)

-------ellipsoid object-------
Properties:
   |    
   |-- actualClass : 'ellipsoid'
   |--------- size : [2 3 1]

Dimensionality: 1000 
Fields (name, type, size, description):
    'Q'    'double'    '[10 10]'    'Configuration matrix'
    'a'    'double'    '[10,1]'    'Center'

Data: 

ellArr(1,1,2)
   |    
   |-- q : [0 0]
   |       -----
   |-- Q : |0|0|
   |       |0|0|
   |       -----
   O

ellArr(2,1,2)
   |    
   |-- q : [0 0]
   |       -----
   |-- Q : |0|0|
   |       |0|0|
   |       -----
   O

You can see how it works for relations by typing

rel=smartdb.relations.DynamicRelation(struct('a',ones(1000,1),'b',ones(1000,1)))

3) Structural outputs in the help headers are not described correctly.

You need to use the following notation

Output:
    SOut: struct[nDims1,nDims2,...,nDimsK] - structural array with the following fields

             alpha: double[1,2,] - ...
             beta: char[1, ] - ...

4) Please use "true" and "false" instead of "True" and "False"

6) >Doesn't hyperplane.display() use strucdisp?
If it does - just make the output look like in "ellipsoid class. 

7) 
>3. I prefer to ask, right way i act or not, before to write tests on my work.

The way is right, you can start writing the tests now.

8) Regarding 4,5 - you are right, I looked in a wrong place.

9) 
>How to understand, that my changes are enough significant?
If you are unsure - assume that they are.

10)

>It conflicts with way to display structural arrays in strucdisp. Strucdisp 
assumes to display first n (10) ellipsoids and then display the rest count of 
undisplayed.
Ok but still such variable should be dispalyed as [3,3,3] array, not as
[3,9] matrix. And even with 10 elemen limitation we can think of another 
example:

ell2 = reshape(ell,[2,2,2]);    

11) You seem to change run_tests function to run the tests from a specific test 
case.
But there is a simpler way -mlunitext.runtestcase function (see 
http://code.google.com/p/ellipsoids/wiki/MLUNITEXT_unit_testing_framework_descri
ption)

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

GoogleCodeExporter commented 8 years ago 
>Also, please use the following format (btw - please rename q into a)
Should i use CubeStruct.displayInternal, make separate displayer for all 
classes or copy code?

Original comment by Alexande...@gmail.com on 18 May 2013 at 9:03

GoogleCodeExporter commented 8 years ago 
Just copy the code, I mentioned displayInternal so that use can use the format 
of resulting text output as etalon.

Original comment by heartofm...@gmail.com on 19 May 2013 at 1:53

GoogleCodeExporter commented 8 years ago 
What have to be written in fields 'size' and 'dimensionality'? Size and 
dimensionality of ellipsoids in array may be different, or not?
Now i just no output this fields.

Original comment by Alexande...@gmail.com on 22 May 2013 at 11:45

GoogleCodeExporter commented 8 years ago 
12) Please implement a static method "fromStruct" in ellipsoid class which is 
symmetrical to
"toStruct". Add a test that 

ell=ellipsoid(...)

ellipsoid.fromStruct(ell.toStruct()) returns true for different combinations
of input arguments and properties (such as "absTol", "relTol").

For this test to pass toStruct should store in the resulting structure
all its properties such as absTol, nPoints3DGrid etc.

Please make toStruct accept an optional "isPropIncluded" argument so that

ell.toStruct(true) produces a structure with all the object fields while
ell.toStruct(false) or ell.toStruct() includes only "shapeMat" and "centerVec" 
fields.

ellipsoid.fromStruct should accept the structures with just two fields 
(shapeMat and centerVec)
in which case the properties are filled with the same values as in constructor.

13) Please make toStruct return 3 outputs - 

  * SDataArr: struct[nDims1,...,nDimsk] - contains ellipsoid fields like absTol, shapeMat etc
  * SFieldNiceNames: struct[1,1] - scalar structure with the same fields as SDataArr. Field values
should contain the nice names like "Q", "a" etc. 
  * SFieldDescr: struct[1,1] - same fields as SDataArr, values contain field descriptions like
"Shape matrix" for shapeMat etc.

14) Make "isEqual" method accept 'isPropIncluded" property (which is false by 
default).
Please note that it should be a property (passed as 
ell.isEqual('isPropIncluded',true)),
not an input argument as in toStruct.

In isEqual please make sure that all these additional properties are compared 
for isPropIncluded=true
(i.e. ellipsoids with different values of absTol are not equal when 
isPropIncluded=true and equal for false)

3) in  testToStruct test 

a) obtainedEllStruct  is structual array, should start with a capital letter
b) Please rename index  into iElem
c) when comparing numeric arrays please write just "isequal(SEll1.Q,SEll2.Q)"

instead of 

compMat=SEll1.Q==SEll2.Q;
isOk=all(compMat(:))

d) compMat, if used somewhere, needs to be renamed to match our 
convention (see the table with example variable names at 
http://code.google.com/p/ellipsoids/wiki/Coding_policy)

e) what have you implemented "isEqual" subfunction for? built-in "isequal" 
function in Matlab
is perfectly capable of comparing the strucures. Also, if you need to compare 2 
structures
with certain precision - use modgen.struct.structcomparevec. in the latter case 
you need to use the second output of structcompare
in mlunit.assert

15) in ellipsoid.display 

a) get rid of the loop by using built-in "mat2str" function and please never 
use names like "index", see the coding convention.
Also, you could have used arrayfun here instead of the loop...
b) use the second and third outputs of toStruct method (see 3)

16) use the second output of "toStruct" in ellipsoid.eq

17) Apply the same changes and impement the same test for hyperplane. 
The tests for both ellipsoid and hyperplane that check "display", "fromStruct" 
and "toStruct"
methods should be implemented withou a copy-pasting. To do that please 
implement 3 separate classes:

ADispStructTC, EllipsoidDispStructTC < ADispStruct, Hyperplane<ADispStuct. 

The same approach is implemented in  
elltool.core.test.mlunit.EllipsoidPlotTestCase and 
 elltool.core.test.mlunit.HyperplanePlotTestCase

both inheriting elltool.core.test.mlunit.BGeomBodyTC

Use those tests as example

Original comment by heartofm...@gmail.com on 23 May 2013 at 2:41

GoogleCodeExporter commented 8 years ago 
>16) use the second output of "toStruct" in ellipsoid.eq
How?

Original comment by Alexande...@gmail.com on 29 May 2013 at 1:55

GoogleCodeExporter commented 8 years ago 
Right now ellipsoid.eq uses the following function

function SComp=formCompStruct(ellObj)
    SComp=struct('Q',gras.la.sqrtmpos(ellObj.shapeMat,...
        ellObj.absTol),'q',ellObj.centerVec.');
end

As you can see "Q" and "q" are hard-coded. Instead you should extract these 
names from the second output of toStruct method.

Original comment by heartofm...@gmail.com on 30 May 2013 at 2:24

GoogleCodeExporter commented 8 years ago 
I've check your last commit and found a few problems:

13.1) in toStruct for ellipsoid (the same is true for hyperplane)

FieldNiceNames = struct('Q', 'shapeMat', 'a', 'centerVec',...
    40  +   'absTol', 'absTol', 'relTol', 'relTol', ...
    41  +   'nPlot2dPoints', 'nPlot2dPoints', ...
    42  +   'nPlot3dPoints', 'nPlot3dPoints');

Actually the nice names are "Q" and "a", not centerVec and shapeMat. Field 
names should correspond to property names while field values - to nice names 
and descriptions.

Also, you seem to fill the structures differently depending on isPropIncluded 
flag. All this flag need to do is to allow adding additional class properties 
into the resulting structures. As to shapeMat and centerVec - they should be 
included regardless of the flag and these should be no difference for these 
fields depending on isPropIncluded flag.

12.1) in fromStruct method please avoid specifying all the properies manually 
like

ellipsoid('abs',SProp.absTol,'relTol,'SProp.relTol,...) 

Instead please use fieldnames function to collect input structure names like 
this:

SProp=rmField(InpStruct,{'shapeMat','centerVec'});
propNameValueCMat=[fieldnames(SProp),struct2cell(SProp)].';
obj=ellipsoid(InpStruct.shapeMat,inpStruct.centerVec,propNameValueCMat{:})

Finally, you need to make both fromStruct and toStruct work with 
multi-dimensional arrays of ellipsoids.
As far as I can see the current implementation works only for scalar objects.

Original comment by heartofm...@gmail.com on 30 May 2013 at 2:43

GoogleCodeExporter commented 8 years ago 
0) Please revert changes to  elltool.test.run_tests and potentially to other 
run_tests functions
1) Apply smart-indent (Ctrl + I) to all your code
2) Fix all matlab code analyzer warnings (see 
http://www.mathworks.co.uk/help/matlab/matlab_prog/check-code-for-errors-and-war
nings.html)
in your code
3) Remove copy-pasting in ellipsoid/hyperplane.toStruct in the following block 
of code

   if (isPropIncluded)
        SFieldNiceNames = struct('normal', 'normal', 'shift', 'shift',...
                                 'absTol', 'absTol');
        SFieldDescr = struct('normal', 'Hyperplane normal.',...
                             'shift', 'Hyperplane shift along normal from origin.',...
                             'absTol', 'Absolute tolerance.');

    else
        SFieldNiceNames = struct('normal', 'normal', 'shift', 'shift');
        SFieldDescr = struct('normal', 'Hyperplane normal.',...
                             'shift', 'Hyperplane shift along normal from origin.');
    end

4) In 
 ellipsoid.eq and hyperplane.eq use the second output of toStruct to name the fields of compare structures.
As a result the field names should be "Q" and "q".

5) In ellipsoid.display

a) rename SdataArray into SDataArray, SfieldNames into SFieldNames etc.

b) the text output generated by the following lines should be generated 

by strucdisp (just form Properties structure with actualClass and size fields)
fprintf('Properties:\n');
fprintf('   |\n');    
fprintf('   |-- actualClass : ''ellipsoid''\n');
fprintf('   |--------- size : ');

6) In ellipsoid.eq and hyperplane.eq 

The following code doesn't make any sense becase for empty array there is no
Q matrix defined so you do not have anything to extract a square root from. 
Prior to Dmitry Khristoforov's change 
isempty was a method of ellipsoid class that return true for ellipsoids without 
any matrix/center specified. Now the name of this method
is isEmpty so when you write isempty you call matlab built-in function that 
returns true for arrays of 0x1x0 size.

Also, please use getAbsTol method for ellFirstArr, not just for the first 
element.
This method returns a scalar value of absTol as a second output.

if (~isempty(ellFirstArr))
    ABS_TOL = ellFirstArr(1).absTol;
else
    ABS_TOL = 1e-6;
end

7) New functionality in strucdisp still is not covered with tests

8) Tests for display methods are too format-sensitive. Instead of comparing the 
output with the expected output symbol by symbol
please find the entries correspoding to the specific field names and then parse 
the following symbol representation of field value.
This way the tests won't be sensitive to the formatting that much.

Also, I still think that it is possible to write generic tests for both 
hyperplane and ellipsoid. The only difference between them
is the field names and their initial values you use to create the objects. Thus 
you can move the body of test methods in ADispStruct test case
(btw please rename it into ADispStructTC) and create the abstract method that 
return field names and their value. Then in the derived 
classes (EllipsoidDispStructTC and HyperplaneDispStructTC)  you can override 
the abstract methods by specifying ellipsoid-specific and
hyperplane-specific field names and values.

9) Once you are done please run elltool.core.test.run_tests first and then 
elltool.test.run_tests to make sure that you haven't broken anything.
There were some tests for ellipsoid.display and we 

10) Add tests for toStruct/fromStruct/eq/diplsay tests for 

a) empty arrays of both ellipsoid and hyperplanes
b) scalar objects (I believe that you have them, just make sure that you have 
all 4 methods covered)
c) multi-dimensional arrays (not just vectors)

Once you are done - please change the status of the issue to "ReadyForReview"

Original comment by heartofm...@gmail.com on 3 Jun 2013 at 12:22

GoogleCodeExporter commented 8 years ago 
>The following code doesn't make any sense becase for empty array there is no
Q matrix defined so you do not have anything to extract a square root from.

There is check for empty arrays. It was written by previous author. With empty 
matrix square root extraction works correctly. I don't see, where is a problem.

Moreover, i didn't find, how to make virtual and pure virtual functions. 
Therefore test for ellipsoid and hyperplane structure conversion still have 
same form.

Original comment by Alexande...@gmail.com on 13 Jun 2013 at 9:10

GoogleCodeExporter commented 8 years ago 
I'm in a middle of review, will write you the results tomorrow morning, now 
about ellipsoid and hyperplane conversion/test similarities... You need to use 
abstract methods and overwrite them in the derived classes - see 
http://www.mathworks.com/help/matlab/matlab_oop/abstract-classes-and-interfaces.
html. Please work on this until I finish my review tomorrow morning.

Original comment by heartofm...@gmail.com on 15 Jun 2013 at 9:03

GoogleCodeExporter commented 8 years ago 
11) In ellipsoid.toStruct please remove centerVec transposition in

 SEll = struct('shapeMat', ellObj.shapeMat, 'centerVec', ellObj.centerVec.');

12) in hyperplane.toStruct please do not use return, use if/then/else 
construct. Also,
please rename hyperplane.isempty method into isEmpty (as this is done in 
ellipsoid class).
In ellipsoid class this was done so that isEmpty method doesn't conflict with 
isempty built-in function
that returns true for empty arrays. In hyperplane you handle "isEmpty=true" 
case separately while in ellipsoid there is no such code?
I would prefer the general-purpose code like in ellipsoid class (but please add 
the tests for both hyperplane
and ellipsoid for empty cases like ellipsoid() and hyperplane). 

13) in ellipsoid.eq in [~, ABS_TOL] = ellFirstArr.getAbsTol; ABS_TOL is not a 
constant so please change its name into absTol.
Constant is something you declare like ABS_TOL=1e-10;

14) Item 4) from the list above for hyperplane is not fixed

16) item 6) from the list above - I'm not sure that eq works correctly for 
empty ellipsoids created by ellipsoid()
command and even if it does - it is just a luch that sqrtmpos works correctly 
with empty input arrays. 
We need the case when ell.isEqual returns true separately in both hyperplane 
and ellipsoid classes.

17) Existing tests for strucdisp are located in 
modgen.struct.test.mlunit_test_mixed, please move your StructDispTC
into modgen.struct.test.mlunit package, move existing tests from 
modgen.struct.test.mlunit_test_mixed into StructDispTC
and modify modgen.struct.test.run_tests. There is a lot of copy-pasting in 
StrucDispTc, please elimitate it via use of 
nested/sub functions. There is also a lot of copy-pasting in 
EllipsoidDispStructTC and HyperplaneDispStructTC, please use
the same technique to get rid of it.

18) Some files contains lines that are longer than 76 symbols, also smart 
indent is not applied to all files you changed/created

19) There is no need to use loops in ADispStructTC, use cellfun instead

20) There is no need to use loops in EllipsoidDispStructTC to create an array 
of ellipsoids, use ellipsoid.fromRepMat.
Please create a similar method in hyperlane (just copy-paste and change 
help-headers and function/variable names) to
get rid of loops in HyperplaneDispStructTC.

Original comment by heartofm...@gmail.com on 16 Jun 2013 at 10:48

GoogleCodeExporter commented 8 years ago 
>In hyperplane you handle "isEmpty=true" case separately while in ellipsoid 
there is no such code?
I would prefer the general-purpose code like in ellipsoid class (but please add 
the tests for both hyperplane
and ellipsoid for empty cases like ellipsoid() and hyperplane). 

I handle empty case in hyperplane separately because i make calculations within 
conversion, and division by empty value will cause error. In ellipsoid it isn't 
nessecary, all works without it.

Original comment by Alexande...@gmail.com on 18 Jun 2013 at 9:24

GoogleCodeExporter commented 8 years ago 
item 14 still not fixed.

Also, the way you implemented the tests in EllipsoidDispTC and HyperplaneDispTC 
doesn't have much to do with the idea I explained in the previous comments. 

All tests should be in the base class (ADispStructTC) while the derivative 
classes should implement protected (not-test methods that provide data for the 
tests from ADispStruct. Please have a look at this example:

classdef ABasicTC

methods (Access=protected,Abstract)
    objArray=createInstance(self,sizeVec)
end

methods 
    function testToFromStruct(self)
        SIZE_VEC_LIST={[10,20],[2,3,4],[1 2 1],[1 1])};
        nSizeListElems=numel(SIZE_VEC_LIST);
        for iElem=1:nSizeListElems
            sizeVec=SIZE_VEC_LIST{iElem};
            objArr=self.createInstanse(sizeVec);
            structArr=objArr.toStruct();
            resArr=objArr.fromStruct(structArr);
            [isOk,reportStr]=resArr.isEqual(objArr);
            mlunitext.assert(isOk,reportStr);
        end
    end
end

end

classdef EllipsidTC<ABasicTC
methods (Access=protected)
    function objArray=createInstance(self,sizeVec)
        SHAPE_MAT=diag([1,2,3]);
        CENTER_VEC=[1;2];
        objArry=ellipsoid.fromRepMat(SHAPE_MAT,CENTER_VEC,sizeVec);
    end
end
end

classdef HyperplaneTC<ABasicTC
methods (Access=protected)
    function objArray=createInstance(self,sizeVec)
        ....
        objArry=hyperplane.fromRepMat(...,sizeVec);
    end
end
end

As you can see the test is located in ABasicTC while the derivate classes just 
encapsulate the class (hyperplane or ellipsoid)-specific logic of creating the 
object arrays of corresponding classes. 

Thus you need to use make the tests polymorphic. And please, try minimize a 
number of case/switch constructs, better use separate tests if you need to or 
just write the tests in plain code like

%check something, case #1
...
%check something, case #2,

...
%check something, case #10,

as opposed to

for iTest=1:10
testSomething(iTest);
end

All these switches just make code less readable and that's all.

Original comment by heartofm...@gmail.com on 21 Jun 2013 at 11:36

GoogleCodeExporter commented 8 years ago 
I don't understand, what is wrong in general with idea of my tests. 
>All tests should be in the base class (ADispStructTC) while the derivative 
classes should implement protected (not-test methods that provide data for the 
tests from ADispStruct.

[x] There is.

>As you can see the test is located in ABasicTC while the derivate classes just 
encapsulate the class (hyperplane or ellipsoid)-specific logic of creating the 
object arrays of corresponding classes. 

[x]There is.

My tests are little bit complicated, that

>objArr=self.createInstanse(sizeVec);
>structArr=objArr.toStruct();
>resArr=objArr.fromStruct(structArr);
>[isOk,reportStr]=resArr.isEqual(objArr);

and every test needs more that one abstract function. Therefore instead one 
'createInstance' i have 'getToStructStruct', 'getToStructObj' e. t. c.

I removed switches and made data transfers in cell vectors, added 'protected' 
qualifier in last commit.

Original comment by Alexande...@gmail.com on 25 Jun 2013 at 4:51

GoogleCodeExporter commented 8 years ago 
>item 14 still not fixed.

In hyperplane.eq there is no hardcoded isung of hyperplane structure view 
fields. Do you mean, i have add a transformation of comparing structure arrays 
into structure arrays with 'nice named' data fields? I did it.

Original comment by Alexande...@gmail.com on 25 Jun 2013 at 5:39

GoogleCodeExporter commented 8 years ago 
The tests look very good now so that is done. 

The only problem I can see now is the following:

ellipsoid.eq and ellipsoid.isEqual basically duplicate each other, we need to 
merge ellipsoid.eq into ellipsoid.isEqual and make ellipsoid.eq call isEqual 
ignoring reportStr output. The difference between eq and isEqual is that eq 
called when you use "==" operation. The same with hyperplane.eq - we need to 
create hyperplane.isEqual, make it return reportStr and accept 'includeProp' 
properties isPropIncluded field.

Also, since the logic ellipsoid.isEqual and hyperplane.isEqual is the same at 
95 percent I suggest you create elltool.core.AuxGenEllipsoid class inherited by 
both ellipsoid and hyperplane classes. AuxGenEllipsoid should inherit handle 
class and should contain isEqualInternal protected method parameterized by the 
funciton that performs post-processing of the structures after toStruct class 
(sqrtmpos in ellipsoid versus no-transformation in hyperplane).

Then ellipsoid.isEqual would look like this;

function [isEqArr,reportStr]ellipsoid.isEqual(self,varargin)

[isEqArr,reportStr]=self.isEqualInternal(self,@postProcSruct,varargin{:});

function SData=postProcStruct(SData)
SData.shapeMat=sqrtmpos(SData.shapeMat);
end
end

Apart from this all seems to be done.

Original comment by heartofm...@gmail.com on 26 Jun 2013 at 4:04

GoogleCodeExporter commented 8 years ago 
1. What to do with 'relTol' optional input argument of eq? Add it to isEqual 
and further? Or remove?

2. Tests fails because they want reportStr output of eq. What to do? I think 
'relTol' they will also want.

Original comment by Alexande...@gmail.com on 26 Jun 2013 at 11:15

GoogleCodeExporter commented 8 years ago 
0) No need to declare a constructor in AuxGenEllipsoid, just remove it.

Please 

1) Declare isEqualInternal as protected method in AuxGenEllipsoid and remove 
commented out lines 
2) Define formCompStruct as Abstract method in AuxGenEllipsoid and rename 
AuxGenEllipsoid into AGenEllipsoid (A for abstract)
3) Move eq and isEqual into AGenEllipsoid. isEqual should call 
self.formCompStruct which will be defined differently in hyperplane/ellipsoid 
4) Add relTol as a property to hyperplane class and make sure it is parsed 
correctly (add a small test). For a reference see ellipsoid.m, lines 192-204. 
Also add getRelTol by copy-pasting its code from ellipsoid class.
5) Remove relTol input argument to eq, eq should use isEqual, isEqual should 
use getRelTol method. Also please remove reportStr output of eq, all calls like 
[isEqArr,reportStr]=ell1.eq(ell2) in tests and other places please just 
redirect to isEqual.

Please do not forget to merge.

Original comment by heartofm...@gmail.com on 27 Jun 2013 at 8:10

GoogleCodeExporter commented 8 years ago 
I did what you asked, except 'elltool.core.test.mlunit.EllipsoidBasicSecondTC'. 
It has test with empty ellipsoids, but now isEqual doesn't accept empty arrays. 
Does i have to make isEqual accept empty arrays or delete this tests?
Other tests sets are not tested yet, and all this tests i cant execute because 
of hardware limitation of my machine.

Original comment by Alexande...@gmail.com on 28 Jun 2013 at 5:48

GoogleCodeExporter commented 8 years ago 
Please make it accept empty ellipsoids, after all isEqual is a method that 
should work as both eq and old isEqual.

After that you can reintegrate but only if ALL tests pass. When reintegrating 
please follow the reintegration procedure described at 
http://code.google.com/p/ellipsoids/wiki/Issues_workflow_and_branching_policy.

Original comment by heartofm...@gmail.com on 28 Jun 2013 at 1:20

GoogleCodeExporter commented 8 years ago 
Have you run all the tests prior reintegration? If so - why there are failures?

Please re-create the branch and fix the broken tests ASAP.

Original comment by heartofm...@gmail.com on 29 Jun 2013 at 2:03

GoogleCodeExporter commented 8 years ago 
No, i have not. I said, i cant perform all amount of tests, my MATLAB falls 
with critical error (send a pic?). I fixed all errors in *.core.* and commited.

Now i fix other errors. I hope, Menshikov reintegration didn't bring new ones. 
Waiting for test results.

Original comment by Alexande...@gmail.com on 29 Jun 2013 at 7:51

GoogleCodeExporter commented 8 years ago

Original comment by Alexande...@gmail.com on 29 Jun 2013 at 7:51

GoogleCodeExporter commented 8 years ago 
Tests passed.

Original comment by Alexande...@gmail.com on 30 Jun 2013 at 2:35

GoogleCodeExporter commented 8 years ago

Original comment by heartofm...@gmail.com on 1 Jul 2013 at 6:25