Opened 16 years ago

Closed 16 years ago

#173 closed defect (fixed)

Optimize return values from member functions of PCA and related classes

Reported by: Jari Häkkinen Owned by: Jari Häkkinen
Priority: major Milestone: yat 0.3 (Public release)
Component: utility Version: trunk
Keywords: Cc:


PCA has many non-optimized returns of vectors, multiple copies are made at return. Maybe const& should be returned?

Change History (8)

comment:1 Changed 16 years ago by Markus Ringnér

Component: classifierutility
Owner: changed from Markus Ringnér to Jari Häkkinen

comment:2 Changed 16 years ago by Jari Häkkinen

Status: newassigned

comment:3 Changed 16 years ago by Peter

Could you remove those trashy get in function names. We expect users to know that they get something when employing a non void function.

Also since we have the eigenvector stored as columns (or rows??) in a matrix it makes no sense to have a funcvtion returning ONE eigenvector. Send back the whole matrix as a const&. But that was probably what you meant in your description :)

comment:4 in reply to:  3 Changed 16 years ago by Jari Häkkinen

Replying to peter:

Also since we have the eigenvector stored as columns (or rows??) in a matrix it makes no sense to have a funcvtion returning ONE eigenvector. Send back the whole matrix as a const&. But that was probably what you meant in your description :)

I am not sure what was in my mind when I created the ticket. I'll go through the interface though. Are we sure we want the whole matrix? Views maybe nice to get?

comment:5 Changed 16 years ago by Peter

From a user perspective it is easier to get a vector i.e. a view. I forgot that thos are available as an inexpensive alternative. Getting the whole matrix is a bit messy since you most likely is interested in a one of the vectors. But...

There are a few buts. A good thing with returning the whole matrix is that it allows passing the matrix further on to another functionality. Remember it is cheaper to create a vector from a matrix than the other way around to build a matrix from a bunch of vectors. Moreover it is confusing whether the function is const or not, if we choose to return a vector (view). We have to return by value (and not reference) since the view is created within the function. Therefore my feeling is that the compiler would allow us to declare the function as const. But is it const? No, not in the Geek orthodox meaning because changing the returned view will change the underlying matrix which is a member in PCA.

This could be a hint that our design of the vector class: to wrap gsl_vector, gsl_vector_view, and const_gsl_vector_view into one class is fundamentally wrong. Perhaps it would be better to separate those into different classes and utilize polymorphism. My gut feeling is that this is just the beginning of a cascade of frustration that this design will result in.

comment:6 Changed 16 years ago by Jari Häkkinen

Since a the eigenvector fetching function is declared const, this will create a vector that is a const view into the underlying matrix of eigenvectors. Trying to change the view will create a nasty runtime abort (which is a feature, trust me). For the sake of discussion the get_eigne... could return a const vector!

I think I can get PCA to return obviously const vectors, document this for the interested user. The question is, which do we prefer? What is the use case? A few vector or all vectors (the matrix of eigenvectors), and we do not have to choose. We can supply both to the user.

I am not sure if polymorphism would solve the problem of const views and non-const views, but this is out of scope of this dicsussion. And isn't the current implementation polymorphism in a non-orthodox C++ way? To quote a few wise men I met in my travels throughout Europe, "This is a very interesting discussion, lets take it off-line somewhere else".

comment:7 Changed 16 years ago by Peter

Ok, thank you for explaining why the world is not as nasty as I (for a minute) thought. And even more thanks for cutting me off like that. The best complimant I've got tonight :) The member matrix in PCA is not const and I thought the non-const constructor was called. However, the matrix is const locally since we have declared the function const. So it not really a problem. And we should of course return a const view to avoid incorrect usage.

I could agree inheritance would probably not solve the issue. How naive!

Regarding returning matrix or vector I am not sure anymore. How is the PCA intended to be used? Typically you don't use the all eigenvectors, but project the data by removing those dimension corresponding to the smaller eigenvalues. Indeed the projection function does not do this but return the data in the new (i.e. rotated) coordinates. Anyway that was just a note.

The eigenvectors are of interest when you wanna go back from the new coordinates to the original ones. For example if trained an ANN in the new coordinate system resulting in a set weights and you wanna transform these weight to the original coordinate system. This is done by a matrix multiplication so if I have to vote here I would vote for returning const matrix&.

comment:8 Changed 16 years ago by Jari Häkkinen

Resolution: fixed
Status: assignedclosed

(In [829]) Fixes #173.

Note: See TracTickets for help on using tickets.