Opened 14 years ago

Closed 14 years ago

#193 closed defect (fixed)

Serious flaw: matrix mutation functionality and view does not work properly

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

Description

Several of the matrix mutable functions ignores the fact that a matrix can be a view. In many cases this is the desired effect but in some cases this is plain stupid.

One example: If a matrix is a view, then a transpose on that matrix will work (somewhat). It will not break the viewed matrix but the next operation on the transposed matrix will created unexpected results since the view is still active whereas the user probably expects to work on the transpose. The question is, should the effects on the transposed matrix be reflected on the underlying viewed matrix.

We need to discuss and define how we want treat view and operator*=, operator=, and more.

Change History (8)

comment:1 Changed 14 years ago by Peter

We took a decision look time ago (Dec 05) that calling operator= on a view (LHS) should not change the viewed matrix/vector. I have experienced some unexpected behaviour because of this and I'm not sure that was a wise decision.

On the contrary, I am more than open to consider a change of this decision. Most important is that we are consistent all trough the class(es), and thereby make it easier to the user to understand the behaviour. I can see three solutions

  1. Views are locked to be const. If this should be an acceptable solution it must be possible to do this locking in a fashion such that breaking the lock is detected by the compiler (???)
  2. Views are true non-const views, i.e., when changing the view also underlying data is changed. This should be true for every function and operator
  3. Lazy implementations, i.e. views are views as long as they are not touched. When they are touched they are mutated into real matrix/vector (expensive copy).

My gut feeling is that 1) is inferior to 2) and 3), simply because locking views to be const is not a restriction in the margin. Also I am not sure it technically is possible to implement without a HUGE amount of man months. If we go for 3), it is tempting to go for the full monthy and implement all copying as lazy copying. A copy is not deep copied until it is necessary. This might be misleading becasue what happens when the underlying data goes out of scope? Underlying GSL matrix cannot be destroyed in destructor but must be dealed in some other way. Ref Counter? With a significant risk of being shallow, I think 2) is the preferable option. Among other reasons it allows code like

 matrix m(10,10);
 vector v(m,0,true) = vector(10,1.0); 

which actually modifies m. This implies that a view is a view also after calling operator= in contrast to the implementation at time being. Should a normal vector/matrix be a normal vector/matrix also after calling operator=? I presume the answer is yes. More interestingly, what happens to those views that are looking in to this normal vector/matrix? They must be modified too, right? But what happens if the dimension of the normal vector/matrix is changed? A related issue is whether the view/normal-state should be copied in copy constructor, in other words , what is displayed from this code:

 matrix(10,10);
 vector v(m,0,true);
 vector v2(v);
 v(0)=2;
 cout << v2(0) << endl;

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

We have decided:

  • Copy constructor results in a normal vector/matrix - never a view
  • in assignment operator underlying data are affected (in case LHS is a view), which implies view/normal-state is not assigned.
  • in assignment dimensions must agree otherwise exception is thrown

comment:3 Changed 14 years ago by Jari Häkkinen

Status: newassigned

comment:4 Changed 14 years ago by Jari Häkkinen

(In [789]) Addresses #193. vector now works as outlined here. Added some functionality. Added a clone function that facilitates resizing of vectors. clone is needed since assignement operator functionality is changed.

comment:5 Changed 14 years ago by Jari Häkkinen

(In [790]) Addresses #193. Fixing doxygen docs, and operator=.

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

(In [791]) Addresses #193. Added missing implementations.

comment:7 Changed 14 years ago by Jari Häkkinen

(In [792]) Addresses #193. matrix now works as outlined in the ticket discussion. Added support for const views. Added a clone function that facilitates resizing of matrices. clone is needed since assignement operator functionality is changed.

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

Resolution: fixed
Status: assignedclosed

(In [793]) Fixes #193. Last test for the new functionality added to matrix.

Note: See TracTickets for help on using tickets.