Implement quadmat function and ellipsoid.quadFunc method and use them everywhere in the toolbox instead of explicit

[pgagarinov]

Deadline - October 28th, by that time the code and all the tests should be in "master" branch

Right now quadratic functions (x-c,Q^{-1}(x-c)),(x-c,Q(x-c)),(x,Q^{-1}x),(x,Qx) are calculated in many places of the toolbox in a different way. This needs to be changed by 1) introducing a single Matlab function that calculates these functions 2) replacing the exact formulas in various places across the toolbox code with the calls to this function.

This function should to be placed next to +gras+geom+ell\rhomat.m function and covered with tests in \products+gras+geom+ell+test+mlunit\SuiteBasic.m test case.

The function should be called quadmat and should accept the following parameters

qMat - the matrix itself xVec - x vector cVec - center (optional, if not specified - zero is assumed) mode - optional regime specifier, can take the following values 'plain' - use Q 'invadv' - use Q^{-1} and calculate it using gras.geom.ell.invmat function that you also need to implement. invmat should have the same body as ell_inv function i.e. just copy content of ell_inv into invmat, cover invmat with tests, remove ell_inv and replace all references to ell_inv with references to gras.geom.ell.invmat 'inv' - use Q^{-1} but instead of calculating Q^{-1} use the algorithm from getPolar nested function of ellipsoid.doesContainPoly method.

quadmat should be covered with all kinds of tests that call it with different combinations of input parameters and also negative tests (calling with incorrect parameters and making sure that an excepted exception is thrown - use runAndCheckError method of mlunitext.test_case, see MLUNITEXT portion of Wiki)

Also, please compare inv and invadv modes on iil-conditioned matrices to see the difference and write a test where invadv gives a better result than inv

Finally once quadmat is fully tested replace calls in all places of the toolbox with the calls to this function.

Here are only a few such places but you need to find all of them, no need to go far - just look at the body of all methods of ellipsoid class.

elltool.core.test.mlunit.MPTIntegrationTestCase.testToPolyhedron \products\elltoolboxcore\@ellipsoid\polar.m \products\elltoolboxcore\@ellipsoid\doesContainPoly.m

Be careful when replacing the calls and make sure that all the tests still pass after the replacement. You can run only the core tests for the ellipsoid class first by calling elltool.core.test.run_tests. If all pass then run elltool.test.run_tests

Finally, create a method that uses quadMat on ellipsoid matrix itself. The name of the method should be ellipsoid.quadFunc

This method needs to be covered with tests. Once this is done again look through the toolbox code to find the places where this method can be used instead of explicit formulas formulas.

[thelastpride]

I have a few questions:

1) invmat should have the same body as ell_inv function i.e. just copy content of ell_inv into invmat, cover invmat with tests, remove ell_inv and replace all references to ell_inv with references to gras.geom.ell.invmat Where can I find all of these references?

2) 'inv' - use Q^{-1} but instead of calculating Q^{-1} use the algorithm from getPolar nested function of ellipsoid.doesContainPoly method.

function polar = getPolar(ell) [cVec, shMat] = double(ell); invShMat = inv(shMat); normConst = cVec'_(shMat\cVec); polarCVec = -(shMat\cVec)/(1-normConst); polarShMat = invShMat/(1-normConst) + polarCVec_polarCVec'; polar = ellipsoid(polarCVec,polarShMat); end What exactly does this function? Is polarShMat the result matrix?

[pgagarinov]

1) Just use "Find files" functionality built into Matlab. All you need to do is to select "Current folder" view and press Ctrl+F.

2) All I meant is that that you need to use this line

"normConst = cVec'(shMat\cVec);"

Thus there should be two methods: a) ell_inv-based b) normConst = cVec'(shMat\cVec); - based

I hope this helps.

[pgagarinov]

What is done so far is fine, just

3) if invmat is used more than once please use import gras.geom.ell.invmat; and then call just "invmat" instead of calling the function by the full name every time.

As I see the task is not complete yet so I'll wait until you have the tests and calls to quadmat implemented.

[thelastpride]

I had a question. Where should be a test that compares the speed, how it should be called, and how to verify the accuracy of his passing?

[pgagarinov]

There should be no any test that compares speed, at least I didn't mention anything about it in the description.

You need 4) tests for quadmat:

quadmat should be covered with all kinds of tests that call it with different combinations of input parameters and also negative tests (calling with incorrect parameters and making sure that an excepted exception is thrown - use runAndCheckError method of mlunitext.test_case, see MLUNITEXT portion of Wiki)

5) Also, please compare inv and invadv modes on iil-conditioned matrices to see the difference and write a test where invadv gives a better result than inv

6) tests for quadFun method.

Which of these items did you refer to?

[thelastpride]

Item 5)

[pgagarinov]

The better B as an approximation of inverse matrix - the less k_1=||A_B-I|| or k_2=||B_A*B-B||. The latter is more related to pseudo-inverse matrix.

You need to come up with an examples when B calculated via invadv provides lesser values of k1 and k2 comparing to inv. Please consider 2 cases: non-symmetrical A and symmetrical A.

Also, apart from k1 and k2 you should consider k3 = ||B-B.'|| for symmetrical A. After all, invadv was introduced to solve a problem with inv(A) being non-symmetrical for symmetrical matrices.

As a result of 5 you need to have a set of A values that demonstrate that invadv perform better than inv in terms of values of k1, k2 and k3.

[pgagarinov]

7) I partially fixed the help header for quadmat, please fix the rest. You will have to make "pull" with rebase (please make sure to set the checkbox "launch pull after rebase").

The idea is that all lines should have an indentation consisting of one space following by some number of tabulations. Also, each parameter should have a type, even character strings.

8) Please do not use square brackets in "if/else if" conditions - they are not necessary, this is not C++ after all. You can write just

if a~=1 end

if strcmp(alpha,plain)

end

9) You should allow for the mode name to be specified in upper case as well, so please use either strcmpi(mode,'plain)

10) I suggest you start using switch as more clear construct instead of if strcmp(... like this

switch lower(mode)

case 'plain',

case 'invadv',

end

lower here makes a convertion to a lower case

11) Please avoid using simple names like "mode" just because there is chance there is function with the same name somewhere out there. 'mode' is fine for the property name so please keep it but it not fine for the variable name, please use calcMode for instance.

[thelastpride]

One more question about 5. Still, I don't understand. My function calculates the value of a quadratic function, the result is the number. To compute k_1, k_2 and k_3 we need the inverse matrix, where to get her, if the function does not return her?

[pgagarinov]

You need k1,k2 and k3 only for invmat testing. Invmat returns a matrix, not a scalar.

quadmat returns a metrics all by itself i.e. k_4 = quadmat can be also considered to be a metrics which you can use along with k1 - k3 to see if the matrix is good or bad. For instance you can use l=e_i (ith orth) and call quadmat with Q= BA*B-B and l.

[pgagarinov]

Just a side note:

norm(invhilb(5)-ell_inv(hilb(5))) - norm(invhilb(5)-inv(hilb(5)))

ans =

  6.82602659217196e-07

[pgagarinov]

On my computer I have

dimVec=2:11;normDiffVec=arrayfun(@(x)(norm(invhilb(x)-ell_inv(hilb(x))) - norm(invhilb(x)-inv(hilb(x)))),dimVec).'

normDiffVec =

                     0
  1.69946440631628e-13
  5.90835231087663e-10
  6.82602659217196e-07
  8.53083103439977e-05
    -0.180588466489505
     -391.253604775645
     -529001.309870989
     -352946866.881819
     -265170786779.823

which means that ell_inv produces a result that is closer to the expected result comparing to just inv. This contradicts to what you said to me on last seminar.

What is a result of the same command on your computer?

[thelastpride]

dimVec=2:11; normDiffVec=arrayfun(@(x)(norm(invhilb(x)-gras.geom.ell.invmat(hilb(x))) - norm(invhilb(x)-inv(hilb(x)))),dimVec).'

normDiffVec =

                     0
 -1.70915041790321e-13
 -1.64791097483565e-10
  8.47763676404306e-07
   0.00014811443481434
   -0.0449945050665518
     -141.992992356689
     -323918.468641859
     -252304672.610552
      260397300120.589

[pgagarinov]

Ok, so I suggest that instead of proving that one method is better than the other you just write a test that makes sure that normDiffVec contains both positive and negative components (i.e. there is no "best" method).

After that I'll take a look at your changes and if everything is ok and all tests pass - we will merge into master.

[pgagarinov]

12) testQuadFunc - all constants like 217 and others need to be declared in the beginning of the function in upper case letters with underscores 13) in testInvMat - dimVec is a constant 14) same problem with constants in testQuadMat 15) A lot of copy-pasting in testQuadMat, please use nested functions ("check" for instance) as I explained on a seminar. 16) In ellipsoid.quadFunc help header needs to be fixed a bit (see my fixes to your help header or a help header for modgen.common.parseparext for instance). The format needs to be followed to the letter.

    function resQuad = quadFunc(self)
        %
        % QUADFUNC - computes quadratic function (x,Q^{-1}x) of given
        %            ellipsoid.
        % 
        % INPUT:
        %   regular:
        %      self: ellipsoid[1,1]
        %
        % Output:
        %   resQuad: double - value of quadratic function
        %
        self.checkIfScalar();
        resQuad = gras.geom.ell.quadmat(self.shapeMat,self.centerVec);
    end
    %

17) Please fix the help header for invmat

[pgagarinov]

16) In function resQuad = quadFunc(self)

• % + % QUADFUNC computes quadratic function (x,Qx) of given
• % QUADFUNC - computes quadratic function (x,Q^{-1}x) of given + % ellipsoid.
• % ellipsoid.
  % % % INPUT: % INPUT:

please change "INPUT" to "Input"

17) No description for qMat input argument 18) Please fix variable names in invmat to match our convention (do not forget to modify variable names in the help header as well).

[thelastpride]

I fixed your comments. Should I make push or wait for Jenkins results?

[pgagarinov]

Let us wait for the tests to pass - 2 hours are not going to change anything. If all tests pass - then proceed according to the procedure I sent via email. The key step is making sure that a tip of master branch didn't move anywhere while you were waiting for the test results. This can happen either because someone committed something to master or reverted some changes during this 2 hours. If this happens - you rebase again (possibly - skipping the reverted commits when rebasing if some changes in master were reverted).

What we need to avoid is broken master branch and incorrect changes spreading to branch of other students via rebase. Then the problem becomes even worse. That is why it is very important that we follow the procedure to the letter and make sure that all the tests pass.

[thelastpride]

https://03670798948362156571.googlegroups.com/attach/67db294cfa9b0/error_fail_list.txt?part=0.3&view=1&vt=ANaJVrHS5GEHBEXfd1BhYZ65tFY2AlDPNJg3Aws_nqgke_-_q3Sbwvo5jT3CQvc3PGYICrEaCjqSN8WFwnx-WoBcHbhynMXWg6TI26V3T-FQhnSxaYky3j8

Please look at this report. These errors should not be associated with my job. What should I do?

[pgagarinov]

These problems have been fixed in master - you need to rebase and wait for the test results. After checking your branch I see that it hasn't been rebased for a while. In case you did rebase - what I see means that you did it wrong. Probably you just didn't do "fetch" prior to rebase. Fetch gets the latest versions of remove branches from github server.

Now I realize that yesterday you asked me if it is ok push into your branch, not master, right? If so - yes, you can push into your branch without waiting for the test results, this is your branch after all. Just no pushes to master without following the procedure.

Thanks.

[thelastpride]

In the afternoon I had some problems, but I fixed them. I have just received a message that all tests passed. Can I merge into master?

[pgagarinov]

You did your rebase incorrectly - please compare all commits that precede your changes in your branch and in master. In master Ilya's commits form "petlya" while in your repository they flat. See?

You need to make you branch exactly as master (graph structure should be 100% the same) + delta where delta is formed from you commits.

There are 2 mistakes that were made: a) incorrect rebase b) push of incorrectly rebased branch to github. I suggest you always check you graph structure by visually comparing it with what you see in master. if there are some differences - you should not push.

[pgagarinov]

Please confirm that you see and understand what is wrong. Then we decide what to do next.

[thelastpride]

I get the gist of the error, but now I don't know how to fix it. I need to undo last changes or make a new rebase?

[pgagarinov]

Ok, here is how this can be fixed. You need to find a tip of your old branch (prior to last incorrectly made rebase). This can be done via RefLog. Right click - >TortoiseGit ->Show RefLog. There you find a commit that corresponded to a tip of your old branch. If you are unsure - you can always right click on a commit and press "show log", if you see a log that looks like a log of your old branch (before rebase) - you found it. After that right click and select "Reset issue_26_NZuyanov" to this and then select "Hard" method. All this will revert your last rebase. Then push to remote/origin/issue_26... so that you have a copy. Then rebase again and this time compare the graphs. If it is incorrect again - you always have a copy at remote/origin/issue_26_NZuyanov as long as you do not push.

[thelastpride]

I made reset but when I try to push I see this message: git did not exit cleanly (exit code 1)

[pgagarinov]

Did you specify "force may discard known changes"?

[thelastpride]

No, this checkbox is unselected

[pgagarinov]

You need to select it because you change a history of your branch. Without this checkbox push won't work. Actually, you need this checkbox selected after each rebase.

[thelastpride]

Thank you very much, it helped. Then, when i RightClick -> TortoiseGit -> Rebase what should I select in field Upstream? FETCH_HEAD, master, remotes/origin/master or something else?

[pgagarinov]

remotes/origin/master but make sure to run fetch first to update remotes

[thelastpride]

" but make sure to run fetch first to update remotes" Could you explain this more? What is FETCH??

[thelastpride]

Now I did the rebase correctly?

[pgagarinov]

Looked at the wrong branch, your rebase is correct. You can merge to master now.

[thelastpride]

Tests successfully passed. Am I right?

[pgagarinov]

Yes, thanks. This task is done. Deleting your branch on github.