Implement a few methods of "ellipsoid" class in "GenEllipsoid" class

[pgagarinov]

Deadline: end of this semester

The goal of this task is to implement a few methods of "ellipsoid" class for "GenEllipsoid" class.

A generalized ellipsoid is a set that can be represented as sum E+L, where L - a subspace of R^n and E is some (may be degenerate) ellipsoid in
an orthogonal complement to L. A generalized ellipsoid doesn't have a configuration matrix in a common sense (by construction) but it does have
a) a basis in L and its complement.
b) a configuration matrix of E (ellipsoid in the orthogonal complement to L).

You will have to refer to the generalized ellipsoid class implementation (elltool.core.GenEllipsoid class)
and its tests in

    elltool.core.test.mlunit.GenEllipsoidTestCase
    elltool.core.test.mlunit.GenEllipsoidPlotTestCase

for a better understanding of how this class works.

Once you gain a sufficient level of confidence in your understanding of GenEllipsoid class please
implement the following methods of ellipsoid class in GenEllipsoid class.
Method names, format of input and output arguments should not be changed.

isbigger
trace
shape
getShape
mtimes
parameters
plus
minus
polar
projection
fromRepMat
volume
maxeig
mineig
empty
trace
toStruct
hpintersect

All these methods should be covered with all kinds of tests that you need to put into
elltool.core.test.mlunit.GenEllipsoidSecTC class. The test coverage should
be very dense (different dimensions, all kinds of generalized ellipsoids, different combinations of
input parameters, both negative and positive tests).

[alextim27]

Do I understand right, methods inv and checkBigger existing in GenEllipsoid path - right, and I don't ave to implement these methods?

[pgagarinov]

Yes, inv doesn't have to be implemented since it already exists in GenEllipsoid.

checkBigger does exist but not isbigger. You need to provide a wrapper implementation of isbigger so that it calls checkBigger internally. The whole idea is to make GenEllipsoid provide exactly the same methods as "ellipsoid" class (with the same names and format of input/output arguments).

[alextim27]

I can't find the ellipsoid.empty implementation, where is it?

[pgagarinov]

empty is a built-in static method, it is implemented for all classes including double, int32 etc. Its implementation is hidden.

[alextim27]

1. Jenkins didn't start even try to test my branch
2. Are the implemented method ok? (Test coverage I will do a bit later)
3. Method shape. In ellipsoid class it returns shape matrix. But GenEllipsoid doesn't have this type matrix. In this case what this method has to do?
4. What can I read to understand what the eigvMat is?
5. GenEllipsoid has elaready contained toStruct method. But it has not the same view as ellipsoid's one. Should I change it? Or write something like wrapper?
   Existing method returns qMat, centerVec and qInfMat. IMHO, it should returns diagMat, eigvMat and centerVec as data.

[pgagarinov]

1. I rebooted Jenkins - looks like there was some kind of software problem. Now everything looks fine.

2. Well, it doesn't look out of ordinary but I need to have a dense test coverage for all these methods until I can say anything. Until then - this is just some code. I suggest that you implement tests for the methods you already wrote first instead of writing new methods without any test coverage.

3. it is getShapeMat method of ellipsoid class that returns a shape matrix. Both getShape and shape methods do something different - please read their help headers. Anyways, for a generalized ellipsoid getShapeMat method should return 3 outputs - the first is eigvMat_diagMat_eigvMat.', the second is diagMat and the third is eigvMat. Same goes for "parameters" method - two additional outputs.

4. The best place is to study code in GenEllipsoid constructor (starting on line 138).
   Here is a brief explanation. If we assume for a second that GenEllipsoid represents a normal ellipsoid than a shape matrix of ellipsoid would be eigvMat_diagMat_eigvMat.'
   However for a generalized ellipsoid there is no such thing as shape matrix so this representation is no longer valid. However we can still represent a generalized ellipsoid if we keep eigenVectors and their lengths separately. Thus in a generic case diagMat is a diagonal matrix that can contain both 0 and Inf on its diagonal. If diagMat(indDim,indDim)=Inf means that eigVMat(:,indDim) belongs to L (see a definition of generalized ellipsoid at the task description). Thus L is formed by those columns of eigVMat that correspond to Inf values on diagMat's diagonal. The rest of eigVMat's columns for an ellipsoid in an orthogonal complement of L.

5. Let us keep toStruct as it is because having qMat and qInfMat is much more human readable than having diagMat and eigVMat. Human-readability is important because toStruct is used by display function and for ellipsoid comparison as well.

[pgagarinov]

I had a look at the tests you added - this is definitely good as the first step but

please

6. Do not call methods as functions - use ellObj.methodName() instead of methodName(ellObj).
7. Do not call constructors without () - use GenEllipsoid() instead of GenEllipsoid.
8. There should be no MLint warnings (there should be a green box in the right upper corner) in all the code you work on. Right now this is not the case. Please fix that.
9. It is not necessary to return self from a test method. Writing a test with an output argument is possible only for a backward compatibility.
10. You should use mlunitext.assert(isOk); instead of mlunitext.assert_equals(true, isOk);
11. Think of a ways to make the code more structured via use of nested/sub-functions. This is not always possible but if there is a possibility - please use it.

Apart from that the test looks good.

[pgagarinov]

44. You shouldn't have copied my code blindly as it contained a few misprints, here is a correct version.

nFields=numel(fieldNameList);
%
for iField=1:nFields
fieldName=fieldNameList{iField};
if isempty(SEll.(fieldName))
SComp.(SFieldNiceNames.(fieldName))=[];
else
SComp.(SFieldNiceNames.(fieldName))=SEll.(fieldName);
end
end

Please verify what I write and say, do not just copy my code blindly.

Anyways, tests fail not because of this problem - the reason they fail is because Q in

SFieldNiceNames = struct('shapeMat', 'Q', 'centerVec', 'q');

is actually a square root of shapeMat. This is because when we compare matrices via structcompare we want to compare something that depends on precision linearly, not quadratically.

The way to fix this is

a) Introduce an additional output to toStruct function that returns a structure with a transformation function for each field, for ellipsoid it will be

SFieldTransformFunc = struct('shapeMat', @(x)gras.la.sqrtmpos(x, absTol), 'centerVec', @(x)x);

b) make isEqualInternal pass this structure into formCompStruct
c) make formCompStruct use

fTransfomr=SFieldTransformFunc.(fieldName);
SComp.(SFieldNiceNames.(fieldName))=fTransfomr(SEll.(fieldName));

instead of just

SComp.(SFieldNiceNames.(fieldName))=SEll.(fieldName);

Also please rename Q in

SFieldNiceNames = struct('shapeMat', 'Q', 'centerVec', 'q');

to "QSqrt" because Q is a square root of shapeMat.

For GenEllipsoid QInf and Q should also be renamed and please use correct transformation functions in GenEllipsoid as well.

Thus test tests do not pass because right now you compare shapeMat's, not sqrtm(shapeMat);

[pgagarinov]

45. Answer to this question depends on what you mean by "the polar of E in orthogonal complement to L". If you mean that polar of E+L is projection of polar(E) to an orthogonal complement to L then I think you are right. But if you mean that polar of E belongs to an orthogonal complement to L then you are wrong (the simplest case if when E is degenerate).

46. Yes, I think this is correct.

[alextim27]

44. SFieldTransformFunc = struct('shapeMat', @(x)gras.la.sqrtmpos(x, absTol), 'centerVec', @(x)x);
    Which absTol it uses, if input of toStruct is array of ellipsoid?
    Maybe return the array of these structures with absTol of each ellipsoid?

[pgagarinov]

absTol from isEqualInternal

[alextim27]

Ok.

47. Can you please have a look at tests EllipsoidBasicSecondTC/testGetProjection and EllipsoidBasicSecondTC/testProjection?

In EllipsoidBasicSecondTC/testGetProjection lines
591/592:
dimVec = [0,0,2,0];
auxTestProjection('getProjection',centVec, shapeMat, projMat, dimVec);
728:
ellArr = ellObj.repMat(dimVec);

Now it's failing, because dimVec contains zeros.
But in previous versions of repMat were jast 3 lines

function resArr=repMat(self,varargin)
sizeVec=horzcat(varargin{:});
resArr=repmat(self,sizeVec);
resArr=resArr.getCopy();

And test passed.
Can you explain please, why it worked before and what I have to do with it now?

[pgagarinov]

Where did the new implementation of repMat come from?

[alextim27]

in 39) You said rename fromRepMatInternal to repMat. I looked at these two functions and decided they are similar. So I deleted old repMat and replaced it with fromRepMatInternal renamed to repMat.

[pgagarinov]

I'm not in front of a computer right now - will take a look later today or tomorrow. But presumably all we need to do is to allow zeros in sizeVec, in which case the result should be the same as built-in "empty" method returns.

[alextim27]

44. Done.
45. Volume is done.
46. I fixed repMat and question is solved
47. EllipsoidTestCase/testMtimes, test in line 1230 is wrong. Because author author calculated result using ' istead of .' with complex matrix.
    If replace ' with .' - there will be error in ellipsoid constructor:
    Error: Norm of imaginary part of sourse object = 0.70252. It can not be more than tolVal = 2.2204e-16, Identifier: GRAS:LA:TRYTREATASREAL:wrongInput:inpMat

[alextim27]

45. I mean that if G = E + L, than P = polar(G) is
    P = {y in L^T | (x, y) <= 1 for all x in E}, where L^T is orthogonal complement to L.
    Is this right?

[pgagarinov]

45. I think so but please write extensive tests, especially for ellipsoids with non-zero centers and make sure that polar for GenEllipsoid = polar of ellipsoid when L is empty and E is not degenerate
    b) that polar (polar(G))=G for a variety of cases

Btw - looking at your changes right now....

[pgagarinov]

49. in formCompStruct there is a misprint in "fTransfomr"

50. repMat error message 'size vector must contain positive integer values.' doesn't
    seem correct as we allow non-negative elements in sizeVec, not just positive - right?

51. Please put functionality of ellipsoid.volume into AEllipsoid.ellVolume and rename ellVolume into volume.
    One of the important properties of protected methods is that they can be overriden so your
    implementation of GenEllipsoid.volume will call AEllipsoid.volume if ~any(qInfMat(:)>0) and
    return Inf otherwise. ellipsoid class in its turn will just inherit an implementation of volume
    method from AEllipsoid. Also I do not think you need to create another object in case ~any(qInfMat(:))>0 like now:

if ellObj.isEmpty()
    simpleEllObj=ellipsoid();
else
    simpleEllObj=ellipsoid(ellObj.centerVec,ellObj.getShapeMat());
end

just call [email protected](self) and that is it.

52. Please make sure there is a negative test for each method of GenEllipsoid you implemented (consider all representative cases of incorrect input arguments).

53. fSingleProjection as a name is good for a variable containing a function handle but not for a function or a method. Please rename fSingleProjection into
    projectionSingleInternal

54. Implementation of GenEllipsoid.isdegenerate doesn't seem to be correct. Generalized ellipsoid E+L is degenerate if and only if L is empty and E is degenerate.
    There is no need to use gras.la.ismatnotdeg(ellObj.getQInfMat(),ellObj.getAbsTol()); especially with absTol to make sure that L is empty. Please make sure that
    you have a proper test coverage for that.

55. Please rename changeShapeMatInternal into "shapeSingleInternal and make sure you use ".'" instead of "'" in
    shMat=0.5*(shMat + shMat');

56. In all the help headers, in hyperplane.fromRepMat for instance

% sizeVec: double[1,n] - vector of size, have
% integer values.

should look like

% sizeVec: double[1,n] - vector of size, have
% integer values.

i.e. lines that follow the line with a parameter name should have an additional leading tabulation. Also please avoid using "n" to denote sizes of input arguments,
names of dimensions should be self-representative like "nDims" for instance.

Compare

% Case3:
% regular:
% normalVec: double[nDim, 1] - normal of
% hyperplanes.
% shift: double[1, 1] - shift of hyperplane.
% sizeVec: double[1,n] - vector of size, have
% integer values.

and

% Case3:
% regular:
% normalVec: double[nDim, 1] - normal of
% hyperplanes.
% shift: double[1, 1] - shift of hyperplane.
% sizeVec: double[1,n] - vector of size, have
% integer values.

the latter example is much more readable.

57. Why do you have to have different implementations of "volume" for ellipsoid and GenEllipsoid classes when implementation of "trace" is common for both of them? Also look like
    there is no any test for "trace" for GenEllipsoid.

58. regarding 48) - I think that the previous implementation of mtimes where conjugation "'" was used instead of transposition ".'" was correct because
    (x,Ay)=(A*x,y) where * is conjugation, not transposition. For real-valued operators transposition = conjugation. So please switch back to "'" in changeShapeMatInternal and revert your changes to the test.

Can you please fix all the problems above, merge to master and only then proceed with implementation of the rest of the methods? Thanks.

[alextim27]

49-51, 53-55, 58) Done.
52,56,57) - for tomorrow.

Can you please look at Jenkins - test of my branch didn't start after today's commits.

[alextim27]

57. The question is in the following:
    Shape matrix of GenEllipsoid now can contain NaN, because if in diagMat there are Inf and in eigvMat there are zeros, than there are NaNs in shapeMat.
    And now there are to ways to calculate shape matrix - first implemented in toStruct, second in getShapeMat. Can you please suggest what way we have to use?

[pgagarinov]

qMat calculated in toStruct is actually a configuration matrix of E in G=E+L representation while qInfMat something like a configuration matrix of L. Both volume and trace for non-empty L should return If so I do not see why we should not use getShapeMat's impementation, we just need to override the implementation of tracer and volume from AEllipsoid class like this:

volumeVal = [email protected](self);
if isnan(volumeVal)
volumeVal=Inf;
end

because in theory the only case when we get Nan is when L is not empty. But please make sure in the tests that you cannot get Nan for a degenerate ellipsoid for instance, we do not want volume return Inf volume for an empty-interior set. Same goes for trace method.

[pgagarinov]

Please note that it is better to merge to master a partial implementation than have an incomplete implementation not merged.

[alextim27]

Yes, sure.
I almost did all items you wrote. There are some headers to fix left. Today will push

[alextim27]

I hope I did every item 49-58)

Can you please have a look again to Jenkins, my branch didn't start to test.
upd. Ok, tests started

[pgagarinov]

Done except for polar and hpintersect methods. These methods will be implemented as part of issue #46