Issue 66 in ellipsoids: Improve display methods for both ellipsoid and hyperplane classes

[ellip...@googlecode.com]

Status: New
Owner: Alexande...@gmail.com
Labels: Type-Enhancement

New issue 66 by heartofm...@gmail.com: Improve display methods for both
ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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




--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[ellip...@googlecode.com]

Updates:
Labels: Priority-High

Comment #1 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

(No comment was entered for this change.)

[ellip...@googlecode.com]

Comment #2 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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].

[ellip...@googlecode.com]

Updates:
Status: Accepted

Comment #3 on issue 66 by Bricker...@gmail.com: Improve display methods for

both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

(No comment was entered for this change.)

[ellip...@googlecode.com]

Comment #4 on issue 66 by Bricker...@gmail.com: Improve display methods for

both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

> b) Logical fields

Logical fields support already exists.

[ellip...@googlecode.com]

Comment #5 on issue 66 by Bricker...@gmail.com: Improve display methods for

both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

How toStruct should be used in hyperplane.display()? Other methods i think
i done.

[ellip...@googlecode.com]

Comment #6 on issue 66 by Bricker...@gmail.com: Improve display methods for

both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

Look what i coded in hyperplane.display().

[ellip...@googlecode.com]

Comment #7 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

1.Logical fields are already supported by strucdisp.

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

[ellip...@googlecode.com]

Comment #8 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Comment #9 on issue 66 by Bricker...@gmail.com: Improve display methods for

both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

Doesn't hyperplane.display() use strucdisp?

[ellip...@googlecode.com]

Comment #10 on issue 66 by Alexande...@gmail.com: Improve display methods

[ellip...@googlecode.com]

Comment #11 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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?

[ellip...@googlecode.com]

Comment #12 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

How to understand, that my changes are enough significant?

[ellip...@googlecode.com]

Comment #13 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

> 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.

[ellip...@googlecode.com]

Comment #14 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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_description)

[ellip...@googlecode.com]

Comment #15 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

> 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?

[ellip...@googlecode.com]

Comment #16 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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

[ellip...@googlecode.com]

Comment #17 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Comment #18 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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

[ellip...@googlecode.com]

Comment #19 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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

How?

[ellip...@googlecode.com]

Comment #20 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Comment #21 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReviewFailed

Comment #22 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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-warnings.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"

[ellip...@googlecode.com]

Updates:
Status: ReadyForReview

Comment #23 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

> 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.

[ellip...@googlecode.com]

Comment #24 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReviewFailed

Comment #25 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReadyForReview

Comment #26 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

> 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.

[ellip...@googlecode.com]

Updates:
Status: ReviewFailed

Comment #27 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Comment #28 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReadyForReview

Comment #29 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

> 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.

[ellip...@googlecode.com]

Updates:
Status: ReviewFailed

Comment #30 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReadyForReview

Comment #31 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReviewFailed

Comment #32 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReadyForReview

Comment #33 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReadyForReintegration

Comment #34 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: New

Comment #35 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Comment #36 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

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.

[ellip...@googlecode.com]

Updates:
Status: ReadyForReintegration

Comment #37 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

(No comment was entered for this change.)

[ellip...@googlecode.com]

Updates:
Status: Reintegrated

Comment #38 on issue 66 by Alexande...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

Tests passed.

[ellip...@googlecode.com]

Updates:
Status: Fixed

Comment #39 on issue 66 by heartofm...@gmail.com: Improve display methods

for both ellipsoid and hyperplane classes
http://code.google.com/p/ellipsoids/issues/detail?id=66

(No comment was entered for this change.)