Fix clang warnings in FactorSVD.

[chrisdembia]

This change is 

[chrisdembia]

In my second commit on this PR, I rearrange constructor initializations.

I tried to match the same order as given in FactorSVDRep.h, but this gave an error (non-warning error):

/home/fitze/repos/simbody/simbody/SimTKmath/LinearAlgebra/src/FactorSVD.cpp:427:46: note: in instantiation of function template specialization
      'SimTK::FactorSVDRep<std::complex<float> >::FactorSVDRep<SimTK::negator<std::complex<float> > >' requested here
template FactorSVDRep<std::complex<float> >::FactorSVDRep( const Matrix_<negator<std::complex<float> > >& m, float rcond );
                                             ^
/home/fitze/repos/simbody/simbody/SimTKmath/LinearAlgebra/src/FactorSVD.cpp:158:5: error: type 'TypedWorkSpace<RType>' does
      not provide a call operator
    singularValues(mn))  {
    ^~~~~~~~~~~~~~~~

So I have to initialize singularValues before inputMatrix. So I rearranged some of the members in FactorSVDRep.h.

[sherm1]

I'm not sure this is right. getRank() is at least logically const. Why do you want to change it?

[chrisdembia]

Its implementation calls non-const methods. If rank == 0, it performs the SVD. See https://github.com/simbody/simbody/blob/master/SimTKmath/LinearAlgebra/src/FactorSVD.cpp#L173

[sherm1]

I'm confused -- how was it compiling at all before then?

[chrisdembia]

Because it wasn't overriding the base class method. It had its own non-const getRank().

[chrisdembia]

If you're talking about the second commit: I don't know, I'm confused!

[chrisdembia]

I'd like to revisit this PR, since I believe it fixes an actual bug.

I'm confused -- how was it compiling at all before then?

If you're referring to the constness of getRank(), here's the answer. FactorSVDRepBase::getRank() was marked const, and FactorSVDRep::getRank() was NOT marked const. Thus, getRank() was not being overridden by FactorSVDRep. I first tried changing the declaration FactorSVDRep::getRank(); to FactorSVDRep::getRank() const;. However, FactorSVDRep::getRank(); is NOT const; it performs the SVD if necessary. Therefore, to properly override getRank(), I made FactorSVDRepBase::getRank() non-const. I think this is better than the alternative of casting away the constness inside FactorSVDRep::getRank().

[chrisdembia]

FactorSVD::getRank() isn't const either: http://chrisdembia.github.io/simbody/classSimTK_1_1FactorSVD.html#a5acc0b06bf2d3be139b49b52f4be0f57

[sherm1]

This isn't right, Chris. In Simbody const is used to indicate the logical meaning of the method, that is, whether it modifies the internal state, not the particulars of the implementation. Since lazy evaluation doesn't change the logical state, getRank() and solve() and the like are const despite the fact that they may perform an internal computation. The test for this (same as for state vs. cache) is whether you could throw away the numerical result and then recreate it with no new information. If so it is just cache and doesn't deserve to change the API.

[sherm1]

Looks to me that FactorLU and FactorQTZ are done correctly, while Eigen and FactorSVD are wrong (in the current code I mean). We should add missing consts to the methods there.

BTW, note that you can't remove const from API methods in a minor release like 3.6 because it is a breaking change to the API. Adding const is OK though because anyone who had writable access can still call the methods.

[chrisdembia]

Ah okay. I think fixing the inheritance is still an improvement over the existing code, which has a bug (the sublcass method is never called). Thanks for your point about breaking changes; is FactorSVDRepBase part of the user interface?

I'm not sure I know enough to add in consts and to do the necessary const-casting. Maybe I'll give it a shot at some point.

[sherm1]

is FactorSVDRepBase part of the user interface?

No.