[Octave-patch-tracker] [patch #8960] Preliminary balancing and balance option for eig

[Carnë Draug]

Follow-up Comment #7, patch #8960 (project octave):

Your many tests are also very much repetitive. Could you change them to reduce
code duplication?

Note that you can create a variable that is shared between all tests with
%!shared blocks, and you can also create functions that are available for the
tests with %!function blocks.  But even with such blocks, there's a lot of
code duplication.  For example, the following simple test could easily replace
more than 300 lines of your tests (and could be extended to remove more):


%!test
%! A = [1, 2; 2, 1];
%! for output_option = {"matrix", "vector"}
%!   for balance_option = {"balance", "nobalance"}
%!     [v, d, w] = eig (A, output_option{:}, balance_option{:});
%!     [v2, d2, w2] = eig (A, balance_option{:}, output_option{:});
%!     assert (isdiag (d))
%!     assert (isdiag (d2))
%!     assert (columns (d), 1)
%!     assert (columns (d2), 1)
%!     assert (v, v2)
%!     assert (d, d2)
%!     assert (w, w2)
%!   endfor
%! endfor


Ask me on IRC if you have issues understanding %!shared and %!function in
simplifying this tests.

But more important, this tests don't actually check the returned values. 
Could you add tests for that?

Another note on coding guidelines, don't use brackets for one line loops and
if. So instead of:


if (nargout == 0 || nargout == 1)
  {
    if (m_flag)
      {
        FloatComplexDiagMatrix d (result.eigenvalues ());
        retval = ovl (d);
      }
    else
      {
        retval = ovl (result.eigenvalues ());
      }
  }
else if (nargout == 2)
  {
    if (v_flag)
      {
        retval = ovl (result.right_eigenvectors (), result.eigenvalues ());
      }
    else
      {
        FloatComplexDiagMatrix d (result.eigenvalues ());
        retval = ovl (result.right_eigenvectors (), d);
      }
   }


You should write:


if (nargout == 0 || nargout == 1)
  {
    if (m_flag)
      retval = ovl (FloatComplexDiagMatrix (result.eigenvalues ()));
    else
      retval = ovl (result.eigenvalues ());
  }
else if (nargout == 2)
  {
    if (v_flag)
      retval = ovl (result.right_eigenvectors (), result.eigenvalues ());
    else
      retval = ovl (result.right_eigenvectors (),
                    FloatComplexDiagMatrix (result.eigenvalues ()));
   }



    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?8960>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/