Refine implementation of elltool.reach.ReachDiscrete to eliminate all the bugs

[GoogleCodeExporter]

The task in tightly related to issue 58. The goal here is to identify the 
implementation problems within the class and have them fixed. Rewriting parts 
of the class code might be necessary. As the first step please start with the 
tests that 

1) compare the internal and external approximations along good directions and 
perform some other consistency checks.
2) compare the results with the ethalon data for simple examples (for which the 
results can be obtained analytically).

I suggest that you move to the approach #2 only once you are done with the 
first approach.
As part of the first approach (item #1 in the list above) you need to use the 
objects of gras.ellapx.smartdb.rels.EllTube and 
gras.ellapx.smartdb.rels.EllTubeProj classes by placing the calculated 
ellipsoidal approximations into these objects as containers. I.e. you need to 
1) Run calculation of reach tube approximations by calling some methods of 
ReachDiscrete class
2) Then extract the paramters of the tubes for both external and internal 
approximations via calling get_ia/get_ea methods
3) Place the parameters for the tubes into EllTube object.
4) Find the projection of the tubes via calling project method of ReachDiscrete 
class
5) Compare the projection with that EllTube.project method returns

Please note that the comparison of support functions along the good directions 
that you did will be done AUTOMATICALLY by EllTube class once you put all the 
data into it (this comparison is implemented in its internal checkConsistency 
method and is called from the constructor)

System parameters should be stored in xml configurations as it is done for the 
continuous system tests. Please refer to item #2 from the description of the 
ticket http://code.google.com/p/ellipsoids/issues/detail?id=37 to understand 
how xml configurations work. You can also talk to Daniil Stepensky and Kirill 
Mayantsev.

Original issue reported on code.google.com by heartofm...@gmail.com on 18 Mar 2013 at 5:56

[GoogleCodeExporter]

Please create one touch test in your test coverage based on each of the 
following files

products\elltoolboxcore\control\test\dtback.m
products\elltoolboxcore\control\test\dtdist.m
products\elltoolboxcore\control\test\dtdist2.m
products\elltoolboxcore\control\test\dtdist2.m
products\elltoolboxcore\control\test\dtreach.m
products\elltoolboxcore\control\test\dtreg.m
products\elltoolboxcore\control\test\econ.m

Original comment by heartofm...@gmail.com on 21 Mar 2013 at 5:33

[GoogleCodeExporter]

As part of this ticket I think we need to split LinSys into LinSysContinous and 
LinSysDiscrete inherited from ALinSys that would hold all the common code. This 
seems to be very simple to do...

Original comment by heartofm...@gmail.com on 21 Mar 2013 at 2:54

[GoogleCodeExporter]

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

• Added labels: Priority-High

[GoogleCodeExporter]

Original comment by kitse...@gmail.com on 27 Mar 2013 at 1:12

• Changed state: ReadyForReview

[GoogleCodeExporter]

1) Please apply the automatic formatting (smart-indent) to all LinSys classes 
and ReachDiscrete (select all, then press Ctrl+I)

2) Please document all methods in all LinSys classes following the convention 
for help header format described at 
http://code.google.com/p/ellipsoids/wiki/Coding_policy. Some inputs can have 
different type use struct[1,1]/ellipsoid[1,1] notation for the type-size 
specification.

3) Please always add the copy-right statement to all classes you create or 
modify significantly

4) Please fix variable names so that they comply with the convention from 
http://code.google.com/p/ellipsoids/wiki/Coding_policy. For instance in ALinSys 
there are variables named like QMat. 

5) Places where the exceptions are thrown strings "LINSYS:" and "linsys :" 
should be removed from both error tags and messages as they are added 
automatically by throwerror function. When you write new code tend to use 
modgen.common.checkvar and checkmultvar  for checking the input arguments for 
consistency as opposed to performing the direct comparisons like "if 
nRows~=nCols then throwerror else ...". The goal is to make the input argument 
checks as compact as possible, checkvar and checkmultvar allows for this.

6) Regarding the test for projection - please re-use the code from 
elltool.reach.test.mlunit.ContinuousReachRegrTestCase that basically makes sure 
that for a system that is a cartesian product of two systems the projection on 
the subspaces corresponding to the multipliers coincides with the reachability 
tubes of for the multipliers (if you have questions - you can ask me or Kirill 
as he is an author of this test case). The idea is to transform 
ContinuousReachRegrTestCase  into AReachRegrTestCase , create two subsclasses - 
ContinuousReachRegrTestCase and DiscreteReachRegrTestCase that would pass 
different factories into AReachRegrTestCase constructor. This will give you a 
test for projection functionality. This is much simpler than the approach I've 
suggested previously. Once this is done the next step should be placing into 
AReach class all the functionality except for the tube calculation. And the 
final step - fixing the calculation logic. 

Original comment by heartofm...@gmail.com on 28 Mar 2013 at 7:49

• Changed state: ReviewFailed

[GoogleCodeExporter]

7) ellbndr_3d and ellbndr_2d methods should be deleted from ReachDiscrete

Original comment by heartofm...@gmail.com on 28 Mar 2013 at 3:54

[GoogleCodeExporter]

8) getNPlot3dPoints and getNPlot2dPoints methods and corresponding properties 
should be in both classes(continuous and discrete) i.e. should be defined in 
the abstract class. 

Original comment by heartofm...@gmail.com on 28 Mar 2013 at 4:14

[GoogleCodeExporter]

What is the best way of creating documentation for base and inherited classes?
Should only methods of interface class be documented?
And what about documenting abstract base class and inherited classes themselves?

Original comment by kitse...@gmail.com on 28 Mar 2013 at 8:45

[GoogleCodeExporter]

I'd suggest to document the interface methods in details and insert the 
references to them in the re-defined (ReachCont... or ReachDiscrete) or 
implemented versions (AReach) of these methods (you just say - see a 
documentation in AReach class or in IReach interface). Not interface methods 
should be documented in the class they are defined in.

Original comment by heartofm...@gmail.com on 29 Mar 2013 at 4:44

[GoogleCodeExporter]

Splitting LinSys class has been finished.

Original comment by kitse...@gmail.com on 30 Mar 2013 at 8:19

• Changed state: ReadyForReview

[GoogleCodeExporter]

I found a few problems - please have them fixed.

Original comment by heartofm...@gmail.com on 1 Apr 2013 at 4:40

• Changed state: ReviewFailed

[GoogleCodeExporter]

Original comment by kitse...@gmail.com on 2 Apr 2013 at 1:18

• Changed state: ReadyForReview

[GoogleCodeExporter]

A few problems found (see comments for your last commit)

Original comment by heartofm...@gmail.com on 2 Apr 2013 at 1:51

• Changed state: ReviewFailed

[GoogleCodeExporter]

Original comment by kitse...@gmail.com on 2 Apr 2013 at 4:41

• Changed state: ReadyForReview

[GoogleCodeExporter]

A few minor problems found - see my comments.

Original comment by heartofm...@gmail.com on 2 Apr 2013 at 4:44

• Changed state: ReviewFailed

[GoogleCodeExporter]

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

• Changed state: ReadyForReintegration

[GoogleCodeExporter]

Actually help is inherited by Matlab very well - try

help('elltool.reach.ReachContinuous.iscut')

Original comment by heartofm...@gmail.com on 4 Apr 2013 at 2:46

[GoogleCodeExporter]

Please do not forget to change the status if you reintegrated - please change 
the status to "Reintegrated"

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

[GoogleCodeExporter]

Original comment by kitse...@gmail.com on 4 Apr 2013 at 10:29

• Changed state: Reintegrated

[GoogleCodeExporter]

Original comment by heartofm...@gmail.com on 4 Apr 2013 at 2:04

• Changed state: New

[GoogleCodeExporter]

Original comment by kitse...@gmail.com on 5 Apr 2013 at 8:49

• Changed state: Started

[GoogleCodeExporter]

display method crashes for empty LinSys objects:

elltool.linsys.LinSysDiscrete.empty(0,0)

self =
Undefined variable dispParamStringsCVec.

Error in elltool.linsys.ALinSys/displayInternal (line 135)
            [s0 s1 s2 s3] = dispParamStringsCVec{:};

Error in elltool.linsys.LinSysDiscrete/display (line 106)
            self.displayInternal(self.DISPLAY_PARAMETER_STRINGS)

Please add a test for this specific case and have the bug fixed. Thanks.

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

[GoogleCodeExporter]

please remove get_mu from ReachDiscrete

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

[GoogleCodeExporter]

1) Please change Access to GetAccess in ReachContinuous:

    properties (Constant, Access = private)
        DISPLAY_PARAMETER_STRINGS = {'continuous-time', 'k0 = ', 'k1 = '}
    end

Please pay attention to red colored marks on the stripe on the right-hand side 
of the main editor window.

2) Methods 

cut 
evolve
getCopy (see how it is done in LinSys classes by Kirill)

needs to be unified. Of there are some discrete-specific fields that need to 
but cut/evolved/copied you can do overwrite these methods in ReachDiscrete 
class implementing some additional behavior like this:

function newReachObj=getCopy(self)

newReachObj=getCopy@elltool.reach.AReach(self);

newReachObj.someDiscreteSpecificField=self.someDiscreteSpecificField;
end

The same should be done with evolve and cut. But honestly I do not see why you 
would need to have the fields (properties) different from the ones in 
ReachContinuous class.

3) The following methods should be moved to AReach with the corresponding 
properties:

 getRelTol
getNTimeGridPoints
getNPlot3dPoints
getNPlot2dPoints
getAbsTol

getProperty(make it protected)

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

[GoogleCodeExporter]

ReachContinuous class doesn't have RelTol and AbsTol properties. It gets it 
from elltool.conf.Properties class. Should ReachDiscrete class do the same?

Original comment by kitse...@gmail.com on 10 Apr 2013 at 4:21

[GoogleCodeExporter]

No, on the opposite, you should move these properties from ReachDiscrete to 
AReach and initialize them from Properties class (as in ReachDiscrete 
constructor).

Original comment by heartofm...@gmail.com on 10 Apr 2013 at 4:45

[GoogleCodeExporter]

Btw - making ReachContinuous use these properties is not your task - I have a 
separate ticket for that. All you need to do is to move them along with their 
getters.

Original comment by heartofm...@gmail.com on 10 Apr 2013 at 4:46

[GoogleCodeExporter]

Don't forget about a test for 
elltool.linsys.LinSysDiscrete.empty(0,0)

Original comment by heartofm...@gmail.com on 10 Apr 2013 at 9:06

[GoogleCodeExporter]

Igor, the idea with calculateData is ok but please keep in mind the following, 
in ReachContinuous class all the calculations are placed into a single method 
called makeEllTubeRel which is used in both the constructor and evolve method. 
Thus I think that there is a simpler way to accomplish the restructuring task:

1) Put everything that is in ReachContinuous class in AContinuous. Everything 
means EVERYTHING except for makeEllTubeRel method. Make the latter protected 
and keep it in ReachContunuous class

2) Delete EVERYTHING in ReachDiscrete class (except for the constants that 
contain display settings) and put the tube calculation logic into 
ReachDiscrete's version of  makeEllTubeRel. That's it. 

Incremental way of modification maybe is good but for me this just seems like a 
waste of time because for each piece of code in ReachDiscrete (except for the 
tube calculation) there is a better code in ReachContinuous.

I do not insist, just want to help you and speed things up. Please let me know 
if it makes sense. Thanks.

Original comment by heartofm...@gmail.com on 14 Apr 2013 at 9:34

[GoogleCodeExporter]

In makeEllTubeRel method you get everything you need for calculating the 
discrete approximation: 

        function ellTubeRel = makeEllTubeRel(self, smartLinSys, l0Mat,...
                timeVec, isDisturb, calcPrecision, approxTypeVec)
            import gras.ellapx.enums.EApproxType;
            relTol = elltool.conf.Properties.getRelTol();
            goodDirSetObj = gras.ellapx.lreachplain.GoodDirectionSet(smartLinSys, timeVec(1), l0Mat, calcPrecision);% YOU NEED TO KEEP THIS LINE, the rest of makeEllTube should be re-done for the discrete systems.

smartLinSys object represents the system dynamics, you can evaluate A(t), 
BPB(t), CQC(t) at any point in time like this:

K>> smartLinSys.getBPBTransDynamics().evaluate(0.3)

ans =

   100     0
     0     4

or

K>> smartLinSys.getX0Mat

ans =

    0.0100         0
         0    0.0100

goodDirSetObj represents the dynamics of good curves l(t)=X^{'}(t,s)l_s where 
l_s is the initial direction at time s. You can think of l_s as of l_0 
specified at time s.

To get initial directions use

K>> goodDirSetObj.getlsGoodDirMat

ans =

     1     0
     0     1

You can calculate l(t) for any l_s from the list of initial directions. In the 
example below 2 means "second" intial direction (i.e. it is a sequential number 
of the inital direction)
K>> goodDirSetObj.getGoodDirOneCurveSpline(2)

ans = 

  gras.interp.MatrixColCubicSpline handle with no properties.
  Package: gras.interp

  Methods, Events, Superclasses

K>> goodDirSetObj.getGoodDirOneCurveSpline(2).evaluate(0.3)

ans =

   -1.8747
    6.4896

You can evaluate all good directions as a matrix like this:

K>> goodDirSetObj.getGoodDirCurveSpline().evaluate(0.4)

ans =

   -3.6554   -3.5531
   17.7654   10.5570

Or you can extract a list of splines for each good curve and the use evaluate 
for each of them:

K>> splineList=goodDirSetObj.getGoodDirOneCurveSplineList();
K>> splineList{1}.evaluate(0.3)

ans =

   -1.0091
    9.3734

Thus the goodDirSetObj and smartLinSys objects contain everything you need for 
the discrete ellipsoidal approximations

Original comment by heartofm...@gmail.com on 15 Apr 2013 at 12:13

[GoogleCodeExporter]

Constructor of ALinSys still has discrFlag, it should be removed from the list 
of input parameters and all the help headers

Original comment by heartofm...@gmail.com on 16 Apr 2013 at 8:56

[GoogleCodeExporter]

What you did so far doesn't make a complete sense to me for one reason:

1) A content ReachContinuous wasn't moved to AReach
2) makeEllTube wasn't made protected, it is still protected

One again, let me describe the idea: all methods of ReachContinuous class will 
work for ReachDiscrete as well, by all I mean ALL i.e. evole, get_ia, get_ea 
etc. I.e. you need to cut and paste everything from ReachContinuous into 
ReachDiscrete

The only methods left in ReachContinuous should be 
a) its constructor
b) makeEllTube

Then in ReachDiscrete you need to re-define makeEllTube

Also, in AReach makeElLTube methods should be declared as Abstract

Original comment by heartofm...@gmail.com on 19 Apr 2013 at 10:03

[GoogleCodeExporter]

I’ll try to explain what I have done in detail.

I haven’t moved ALL the methods from ReachContinuous to AReach because 
methods such as getBackwardCMat, rotateEllTubeRel and part of prepareSysParam 
intended for ReachContinuous only. They are used during creation of ellTube and 
they are based  on idea that backward reachability set may be built using the 
same formulas as forward but with some signs changed from plus to minus. The 
problem is that this approach is wrong for discrete-time systems. So I thought 
that these methods should stay in ReachContinuous. prepareSysParam method was 
splitted and common code for discrete- and continuos-time systems was placed 
into AReach.

The reason I haven’t moved evolve methods from ReachContinuous to AReach is 
that I haven’t checked yet if there is any problems like I have mentioned 
before.

As you can see all the rest methods were moved to AReach.

I also created discrete-specified method makeElLTube in ReachDiscrete.

I didn’t get your instructions:
“makeEllTube wasn't made protected, it is still protected” and “AReach 
makeElLTube methods should be declared as Abstract”.
For now there are only makeElLTube methods in ReachContinuous and ReachDiscrete 
classes both. Should I also create makeElLTube in Abstract? And what it should 
be intended for?

Original comment by kitse...@gmail.com on 20 Apr 2013 at 9:38

[GoogleCodeExporter]

Thanks for explanation, this makes much more sense now.

1) 

>They are used during creation of ellTube and they are based  on idea that 
backward >reachability set may be built using the same formulas as forward but 
with some >signs changed from plus to minus. The problem is that this approach 
is wrong for >discrete-time systems.

Standard representation looks like this:

x[k+1]  =  A[k] x[k]  +  B[k] u[k]  +  G[k] v[k]

We can change it to 

x[k+1]-x[k]  =  (A[k]-I) x[k]  +  B[k] u[k]  +  G[k] v[k]

which is a finite-difference representation of continuous case.

If we parameterize makeEllTube by (A-I), B, G I do not see why the same 
approach cannot work.... If you disagree - please explain why.

>I didn’t get your instructions:
>“makeEllTube wasn't made protected, it is still protected” and “AReach 
makeElLTube >methods should be declared as Abstract”.
>For now there are only makeElLTube methods in ReachContinuous and 
ReachDiscrete >classes both. Should I also create makeElLTube in Abstract? And 
what it should be >intended for?

Both ReachContinuous and ReachDiscrete classes have common code. This code 
should be be in AReach and it should use makeEllTube which is different for 
each class. In OOP this can be done via declaring makeEllTube method as 
abstract in AReach class (see 
http://www.mathworks.com/help/matlab/matlab_oop/abstract-classes-and-interfaces.
html?searchHighlight=abstract+method#brjh119) so that the rest of the code in 
AReach can call it not having a specific implementation. Both ReachDiscrete and 
ReachContinous would overwrite this method. To overwrite a method it needs to 
be either protected or public. This in AReach this method needs to be declared 
as (Abstact,Access=protected)

Also, apart form evole Vitaly is working on refine method which also calls 
makeEllTube. The idea is to centralize all the logic which sets ReachDiscrete 
and ReachContinuous apart into a single method - makeEllTube and make the rest 
of the code common.

Original comment by heartofm...@gmail.com on 20 Apr 2013 at 10:26

[GoogleCodeExporter]

Do you have any references to papers with formulas for approximation based on 
finite-difference representation?

For example, there is a paper on Ellipsoidal Techniques for Reachability 
Analysis of Discrete-Time Linear Systems by Alex Kurzhanskiy and Pravin Varaiya 
where approximation for reachability set are built based on standard 
representation.

And for backward-reachability set standard representation looks like this:
x[k] = inv(A[k]) (x[k+1] – B[k] u[k] – G[k] v[k]).

So there is difference in the formulas that are reflected in the code.

Original comment by kitse...@gmail.com on 20 Apr 2013 at 2:13

[GoogleCodeExporter]

I do not know any because finite-difference representation is standard 
representation are the same. I'm not trying to say that we can do thing exactly 
as in continuous case but for both continuous and discrete cases the 
solvability domain is the reachability domain for some other system (for 
continuous case it is sufficient change make a time variable substitution, for 
discrete case we need to change A,B,G and make a time substitution) thus the 
the idea is the same:

we have 3 pieces of functionality (transformations Rho, Phi, Xi) for both 
discrete and continuous cases that are different:

a) forward-time tube calculation algorithm Rho such that for a system S the 
resulting reach set is Rho(S)

b) transformations Phi and Xi such that for system S finding the opposite time 
reach tube for A is the same as Xi(Rho(Phi(S))

Thus

Rho is represented by makeEllTube method. 

for continuous case we have Xi is represented by rotateEllTube method and Phi 
is represented by prepareSysParam (if timeVec(1) > timeVec(2) comparison, line 
498).

All this means that it is possible to create 3 separate methods for Phi, Xi, 
Rho and make these methods abstract and protected in AReach. Thus the only 
methods defined in ReachContinuous/Discrete would be these 3 methods and the 
constructors, the rest of the code can be made common. Maybe be you need to 
make small changes to ReachContinuous to make all this happen but still, 95% 
(if not 100%) of work for ReachContinuous is already done.

Original comment by heartofm...@gmail.com on 20 Apr 2013 at 4:38

[GoogleCodeExporter]

It seems that to follow your approach it’s needed to change (A, B, G) system 
to (inv(A), inv(A) * B, inv(A) * G). The question is how it is possible if A, 
B, G may be represented as time-dependent cells of strings.

Original comment by kitse...@gmail.com on 20 Apr 2013 at 5:45

[GoogleCodeExporter]

Very easily, we have a big library of various stuff for such things:

import gras.mat.symb.MatrixSymbFormulaBased;
import gras.mat.symb.MatrixSFBinaryProd;
import gras.mat.symb.MatrixSFTripleProd;
%
aCMat={'cos(t)','sin(t)';'-sin(t)','cos(t)'};
bCMat={'sin(t)','0';'cos(t)','1'};
aMatFcn=MatrixSymbFormulaBased(aCMat);
bMatFcn=MatrixSymbFormulaBased(bCMat);
%
compOpFact=gras.mat.CompositeMatrixOperations();

aInvFcn=compOpFact.inv(aMatFcn);
multFcn=compOpFact.rMultiply(aInvFcn,bMatFcn);
%
timeVec=0:0.1:10;
fcnVec=multFcn.evaluate(timeVec);
disp(size(fcnVec));

For more information please study the tests in gras.mat package. 

Using the composite operations factory you can combine the objects returned by 
methods like smartLinSys.getBPBTransDynamics() etc.

Original comment by heartofm...@gmail.com on 20 Apr 2013 at 6:49

[GoogleCodeExporter]

After matrix production we get an object multFcn of some class. But it is 
needed to be an cell matrix to pass it to getSmartLinSys that uses 
LReachProblemDynamicsFactory.createByParams function.

Is it possible to transform result of matrix production multFcn to cell matrix?

Original comment by kitse...@gmail.com on 20 Apr 2013 at 7:42

[GoogleCodeExporter]

there is simpler way.
LReachProblemDynamicsFactory.createByParams returns objects that implement

gras.ellapx.lreachuncert.probdyn.IReachProblemDynamics interface. This 
interface is designed to return the dynamics of A(t), B(t)P(t)B(t).' and 
C(t)Q(t)C(t).' as  IMatrixFunction objects. You can easily create your own 
implementation of IReachProblemDynamics interface that returns

inv(A(t)), 

inv(A(t))* B(t)P(t)B(t).'* inv(A(t).'), 

inv(A(t))* C(t)Q(t)C(t).'* inv(A(t).') 

(C is the same s G)

using the following methods of gras.mat.CompositeMatrixOperations():

inv
transpose
rMultiply

Also 

IReachProblemDynamics returns the dynamics of the system transition matrix 
X(t,t0) but after commit of Yuri Admiralsky (which should happen 
today-tomorrow) this will be gone.

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

[GoogleCodeExporter]

It seemed that your idea was to use code of reachability set computing for 
backward-reachability computing too by calling MakeEllTube function with 
different parameters.

So I don't see how adding this new functions you suggested will make any 
difference. I already have different code for backward and forward reachability 
sets computing.

If it is allowed not to unify this code I can manage without suggested methods. 
And if there is need of unification they don't seem to be useful.

So I'm asking if it is necessary to reuse code of forward reachability set 
computing. As I already told ReachContinuous uses prepareSysParam method for 
cell transformation of matrixes A, B, G be concatenating it with minus. After 
that this matrixes are used to create smartLinSys.

So I asked if there is possibility to get inv(A), inv(A) * B, inv(A) * G 
matrixes as cells because it seems that you insist on using code of 
ReachContinuous.

Original comment by kitse...@gmail.com on 21 Apr 2013 at 1:47

[GoogleCodeExporter]

You need to re-use the code of such methods like projection, evolve and refine 
(done by Vitaly in his branch). If you can do it -fine but I'm not ok with 
rewriting this methods.  I suggest a way to re-use the code in this methods by 
centralizing all the differences in 3 methods.

>So I asked if there is possibility to get inv(A), inv(A) * B, inv(A) * G 
matrixes as >cells b
No, you can get them as cells using the existing functions and it is not an 
efficient way to go in my opinion. You need to have a slightly different 
implementation of this method for the discrete case and this method will be 
part of Phi transformation. 

prepareSysParam for the discrete case doesn't contradict my idea of reusing 
ReachContinuous code but nobody told that you do not have to change a single 
line of code in ReachContinuous...

Original comment by heartofm...@gmail.com on 21 Apr 2013 at 1:55

[GoogleCodeExporter]

An alternative approach that I can see is creating 

makeForwardEllTube and makeBackwardEllTube methods, tweaking evolve, refine and 
project methods in ReachContinuous use these methods instead of just 
makeEllTube.

In ReachContinuous makeForwardEllTube and makeBackwardEllTube will use 
makeEllTube while in ReachDiscrete they will have separate implementations 
(like now). 

Of course both makeForwardEllTube and makeBackwardEllTube should be protected 
and abstract in IReach. Please let me know if it is easier for you to 
implement. 

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

[GoogleCodeExporter]

This approach seems to be accomplishable.

But what is the need to separate makeEllTube method into forward and backward? 
Isn't isBack field enough?

Original comment by kitse...@gmail.com on 21 Apr 2013 at 6:36

[GoogleCodeExporter]

Let me demonstrate by simple example. Right now in constructor of 
ReachContinuous there is the following code:

            self.ellTubeRel = self.makeEllTubeRel(smartLinSys, l0Mat,...
                [min(timeVec) max(timeVec)], isDisturbance,...
                relTol, approxTypeVec);
            if self.isbackward()
                self.ellTubeRel = self.rotateEllTubeRel(self.ellTubeRel);
            end

once you introduce forward and backward methods it should look like this:

if self.isbackward()
            self.ellTubeRel = self.makeBackwardEllTubeRel(....);

else 
           self.ellTubeRel=self.makeForwardEllTubeRel(....);
end

Same in evolve method and others, you just call either makeForward or 
makeBackward 

rotateEllTubeRel methods should be called ONLY from makeBackward method i.e. 
the latter should hide rotateEllTubeRel from other stuff. 

Also I suspect that you need to split prepareSysParam into backward and forward 
versions as well. 

Generally speaking the idea of this approach is put backward and forward 
functionality into separate methods while rho/phi/xi approach is about have the 
separate methods for transforming a forward problem into a backward problem and 
vice-versa. 

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

[GoogleCodeExporter]

I'll wait for a working implementation... Until then reviewing the code doesn't 
make much sense to me...

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

[GoogleCodeExporter]

1) Fix all mlint warnings in your code
2) Eliminate copy-pasting in calculateApproxShape 
  and calculateApproxShapeWithDist
3) Apply smart-indent to all your code (ctrl+I).
4) Remove all calls of 

elltool.conf.Properties.getRelTol/getAbsTol in ReachDiscrete - you need to use 
absTol/relTol properties of reachDiscrete class

5) Both ReachContinuous and ReachDiscrete constructors should use the 
same code for parsing of input parameteres.

6) Remove OptStruct in both ReachDiscrete and ReachContinuous constructors, 
this structure is replaced with properties.

7) ReachDiscrete constructor's healp header should be refined:

a) You need to use 'regular' keyword for regular input paramters,
 optional for optional ones and properties for properties (see example at
http://code.google.com/p/ellipsoids/wiki/Coding_policy)

b) Please document all properties/input parameteres (type, name etc) and
remove the legacy ones (OptStruct for instance).

8) Please rename QArrayList into qArrayList - only structure names start with a 
capital letter.

9) ell_regularize needs to be removed as we discussed

10) Please make all tests currently used for ReachContinuous pass.

11) We should not use Properties.setIsVerbose is our classes because

something might crash and the verbosity level will remain changed.

You could use something like this

            isVerbose = Properties.getIsVerbose();
            Properties.setIsVerbose(false);

try

    %do somethign here;

Properties.setIsVerbose(isVerbose);
catch meObj

Properties.setIsVerbose(isVerbose);
rethrow(meObj);

end

but still chanign the system-wide setting is not a good idea. So please
remove Properties.setIsVerbose calls altogether. You can use 
Properties.getIsVerbose();

and regular log4j logging based on this like:

if isVerbose
logger.info('some message');

end

but all this is completely up to you. 

12) Variable names in AReachDiscrBackwardDynamics and other Dynamics classes do 
not comply with our naming convention:

data_Xtt0 (underscores, no suffix etc), BPBTransDynamics (not a structure, why 
capital letter etc).

Original comment by heartofm...@gmail.com on 7 May 2013 at 4:11

[GoogleCodeExporter]

2) in ReachDiscrete, lines 135 - 165

There are only two differences between External and Internal cases:

a) minkmp_ia vs minkmp_ea
b) order of matrices bpbMat and gqgMat

Please create a generic sub/nested function parameterized by mink function
and these two matrices and use it to eliminate the copy-pasting

Also, it looks like isDistrib=false is the same as isDistribut=true
for gqgMat=O. Then the question is - why don't we just remove isDistrib flag
and assume that no disturnable means gqgMat=O?

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

[GoogleCodeExporter]

13) In GoodDirsDiscrete

please rename dataXtt0 into dataXtt0Arr (Arr is for 3d arrays, Mat for matrices 
etc)

14) Everywhere in Reach classes please rename 
smartLinSys into probDynObj and getSmartLinSys into getProbDynamics

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

[GoogleCodeExporter]

As we deleted OptStruct how should I get 'minmax' field? Should it be added 
into Properties?

Original comment by kitse...@gmail.com on 17 May 2013 at 12:05