Loading ...
Sorry, an error occurred while loading the content.

Re: [ublas-dev] Re: [Boost-Users] ublas matrix slice bug?

Expand Messages
  • jhr.walter@t-online.de
    Hi Michael, you wrote: [snip] ... We are talking about the intricacies of the language! I m trying to get that stuff compiling on a compliant compiler. For the
    Message 1 of 19 , Apr 8, 2003
    • 0 Attachment
      Hi Michael,

      you wrote:

      [snip]

      > Since swap is usually assumed to be a fast basic operation even a small
      > overhead associate with the copy constructor would be a very bad thing.

      We are talking about the intricacies of the language! I'm trying to get that
      stuff compiling on a compliant compiler. For the interface using references,
      Intel 7.0 (EDG frontend) accepts in non strict mode and warns in strict
      mode, GCC fails.

      > Certianly compilers like gcc2.95 and VC6/7 will no optimise this away.

      Maybe.

      > Can I vote that the original definition is used.

      If you show us how to compile the OP's sample then.

      > As noted by other the
      > STL decleration for the generic swap is
      >
      > void swap(Assignable& a, Assignable& b);
      >
      > it would seem very bad to use something different where the sematics are
      > similar but subetly changed!

      That's not the intent. I only want to get that working.

      Best,

      Joerg
    • jhr.walter@t-online.de
      Hi Kresimir (hi, all), ... Yep. We re talking about surprisingly dark corners of the language once again, I assume. ... Yep, that s how I envisioned it. ...
      Message 2 of 19 , Apr 8, 2003
      • 0 Attachment
        Hi Kresimir (hi, all),

        you wrote:

        > >Is there not an efficiency issue with the additional copy constructors
        > >necessary for the pass by value?
        > >Or are you assuming (hopeing) the compiler will completely optimise
        > >these away.
        > > If not it seems a high price to pay for the swap operation.
        >
        > matrix_row<> object contains only a reference to the matrix
        > and a row index, so copy constructor is not very expensive.
        > Instead of copying pointer (which reference conceptually is),
        > pointer and (unsigned) integer are copied. But, on the other
        > hand, in `swap()' there is only indirection -- instead of
        >
        > reference parameter -> matrix reference -> matrix row
        >
        > there is only
        >
        > matrix reference (i.e. local copy) -> matrix row
        >
        > (Maybe smart compiler can optimize away double reference;
        > I don't know -- one should make some benchmarks.)

        Yep. We're talking about surprisingly dark corners of the language once
        again, I assume.

        > Peter Schmitteckert wrote:
        >
        > > Does it make sense to have a call by value for swap ?
        > > Why should I want to swap local copies of matrix.
        >
        > They are not `local copies of matrix'. Conceptually,
        > they are local copies of the references to the parts
        > of the matrix. As both the original and the copy refer
        > to the same matrix row, `swap()' works. See below.

        Yep, that's how I envisioned it.

        > > I expect from a call by value that the original variables are not
        > > changed, since the function is working on copies.
        >
        > Indeed, references (or, in the lack of better word, `reference
        > objects') are not changed, but objects to which they refer are.

        Yep.

        > If you have function
        >
        > void swap (int& i, int& j);
        >
        > `i' and `j' are references. Now, after
        >
        > int ii = 5, jj = 6;
        > swap (ii, jj);
        >
        > `i' and `j' are not changed. But (if `swap()' has expected
        > semantics ;o) `ii' will contain 6, and `jj' 5.
        >
        > Similar thing happens with `swap()' for matrix rows
        > (or other proxies):
        >
        > void swap (matrix_row r1, matrix_row r2);
        >
        > If we have matrix `m' which contains elements
        >
        > 11, 12, 13, 14
        > 21, 22, 23, 24
        > 31, 32, 33, 34
        > 41, 42, 43, 44
        >
        > and we write (simplified syntax, without templates):
        >
        > matrix_row mr0 (m, 0), mr2 (m, 2);
        >
        > then `mr0' refers to the first row of `m', that is, it points
        > to the region of memory where elements `11, 12, 13, 14'
        > of `m' are stored [in other words, `mr0' will not contain (new)
        > vector with `11, 12, 13, 14' stored somewhere else].
        > Similarly for `mr2':
        >
        > 11, 12, 13, 14 <-- mr0
        > 21, 22, 23, 24
        > 31, 32, 33, 34 <-- mr2
        > 41, 42, 43, 44
        >
        > Now, let's call swap:
        >
        > swap (mr0, mr2);
        >
        > `r1' now contains copy of `mr0', that is, it points to the region
        > of memory where elements `11, 12, 13, 14' of `m' are stored
        > (`11, 12, 13, 14' are not copied). Similarly, `r2' contains copy
        > of `mr2' and therefore points to the third row of `m':
        >
        > 11, 12, 13, 14 <-- r1
        > 21, 22, 23, 24
        > 31, 32, 33, 34 <-- r2
        > 41, 42, 43, 44
        >
        > After `swap()' `r1' and `r2' are not changed -- `r1' still refers
        > to the first row of `m' and `r2' to the third row (well, in fact,
        > when we leave `swap()', `r1' and `r2' do not exist anymore,
        > but I hope that you understand what I am trying to say ;o).
        > Neither `mr0' and `mr2' are changed -- `mr0' refers to the first
        > row of `m' and `mr2' to the third row. But, contents of these rows
        > are swaped:
        >
        > 31, 32, 33, 34 <-- r1, mr0
        > 21, 22, 23, 24
        > 11, 12, 13, 14 <-- r2, mr2
        > 41, 42, 43, 44

        I'd love to be able to explain it like you.

        Thanks,

        Joerg
      • jhr.walter@t-online.de
        Hi Kresimir, ... values, ... This is *the* example IMHO. ... Earlier versions of view s swap() used swap_ranges() as far as I remember. ... ... That s
        Message 3 of 19 , Apr 8, 2003
        • 0 Attachment
          Hi Kresimir,

          you wrote:

          > >>After `swap()' `r1' and `r2' are not changed -- `r1' still refers
          > >>to the first row of `m' and `r2' to the third row [...]
          >
          > > With that semantic, one should use swap() wit copy semantics,
          > > since r1 and r2 are not changed.
          > > I thought that the references should be exchanged, not the matrix
          values,
          > > which is also a reasonable semantic of swap.
          >
          > Obviously, there are two possible semantics of swap for proxy
          > classes -- swapping references and swapping actual elements.
          >
          > Question is whether the exchange should reflect on the underlying
          > matrix or not. Is it possible to write LU decomposition with pivoting
          > if the contents of the rows are not exchanged?

          This is *the* example IMHO.

          > > STL/algorithm has an iter_swap synonumous to swap( *a, *b) to change
          > > the content of iterators, aren't the matrix_rows in that sense similar
          > > to iterators? Shouldn't we use swap_elements or something similiar
          > > instead?
          >
          > This question can certainly be discussed. Good names are important.

          Earlier versions of view's swap() used swap_ranges() as far as I remember.

          > > From swap I assume that it's cheap, just exhanging the description, not
          > > the data, your swap actually exchanges the elements of the underlying
          > [As a side note, it is not `my' swap. I am not the author of ublas.]

          <giggle>

          > > matrix, which can be expensive.
          >
          > This makes sense if one writes
          >
          > matrix m;
          > matrix_row mr1 (m, 1), mr2 (m, 2);
          > swap (mr1, mr2);
          >
          > and then, when he needs exchanged rows, uses only mr1 and mr2,
          > but if he is also aware that m is same as before swap.
          >
          > But this thread began with the question why
          >
          > swap (row (m, 1), row (m, 2));
          >
          > when swap's signature is
          >
          > void swap (matrix_row&, matrix_row&);
          >
          > cannot be compiled (with standard conforming compiler). If it can
          > be compiled, and if the semantics of swap is as you suggested,
          > what's the use of it? Temporaries returned from row() are exchanged,
          > this is not reflected on the underlying matrix, temporaries are
          > destroyed after the return from the swap -- so nothing happens.

          That's not enough.

          Best,

          Joerg
        • jhr.walter@t-online.de
          Hi Stephan, ... Exactly. ... But nevertheless it s pretty surprising, that one needs to swap these by value ;-) Best, Joerg P.S.: New download is available
          Message 4 of 19 , Apr 8, 2003
          • 0 Attachment
            Hi Stephan,

            you wrote:

            > Your changes are very welcome! The new code does require far less local
            > variables and is much more compact on the user side.
            >
            > Correct me if I am wrong (I did not dig into the internals of ublas),
            > but does not swap() rather change the matrix itself and not the
            > matrix_row representations?

            Exactly.

            > If this is the case, then the whole topic
            > of "not changing temporaries in not-const references" is of course
            > true, but does not reflect the situation, since not the temporaries,
            > but the matrix, on which the temporaries rely, is changed. Then the new
            > signatures do reflect the situation much better.

            But nevertheless it's pretty surprising, that one needs to swap these 'by
            value' ;-)

            Best,

            Joerg

            P.S.: New download is available at
            groups.yahoo.com/group/ublas-dev/files/ublas_2003_04_08.zip

            P.P.S.: Basically regression tested.
          • jhr.walter@t-online.de
            Hi Julius, you wrote: [snip] ... non-const ... ... code like this ... They ll have their usual difficulties then (i.e. checking every bullet of the
            Message 5 of 19 , Apr 8, 2003
            • 0 Attachment
              Hi Julius,

              you wrote:

              [snip]

              > > >>question is the following: Is it intentional, that it
              > > >>is not possible to compile:
              > > >>
              > > >>#include <boost/numeric/ublas/matrix.hpp>
              > > >>#include <boost/numeric/ublas/io.hpp>
              > > >>
              > > >>int main () {
              > > >> using namespace boost::numeric::ublas;
              > > >> matrix<double> m (4, 4);
              > > >> for (unsigned int i = 0; i < m.size1 (); ++ i)
              > > >> for (unsigned int j = 0; j < m.size2 (); ++ j)
              > > >> m(i, j) = 4*i+j;
              > > >>
              > > >> std::cout << m << std::endl;
              > > >>
              > > >> swap(row(m,0), row(m,2));
              > > >>
              > > >> std::cout << m << std::endl;
              > > >>}
              > > >
              > > > No. This one compiles with ICC 7.0 and fails with GCC 3.2.1.
              > >
              > > It also fails with como. As it should.
              >
              > Compiles with VC++ 7, boost 1_30. VC7 allows temporary structs as
              non-const
              > reference parameters, but no ints.

              <giggle>

              > I seem to remember a rumour that the Standards
              > committee considers a change in that direction, precisely to make generic
              code like this
              > here possible (?).

              They'll have their usual difficulties then (i.e. checking every bullet of
              the standard against this change ;-).

              > > > This is the old problem of binding references to temporaries:
              > > > parameters of `swap()' are *non-const* references, and they
              > > > cannot be bound to temporaries returned by `row()'.
              > > >
              > > > It seems that `project()' raises its ugly head again :o(
              > >
              > > Not sure about that. Maybe swap()'s signatures are inadequate for the
              view
              > > classes. If I change matrix_row<>'s swap() member signatures from
              > >
              > > void swap (matrix_row &mr);
              > > friend void swap (matrix_row &mr1, matrix_row &mr2);
              > >
              > > to
              > >
              > > void swap (matrix_row mr);
              > > friend void swap (matrix_row mr1, matrix_row mr2);
              > >
              > > the following test program compiles on GCC 3.2.1 and Intel 7.0:
              > >
              > > ----------
              > > #include <boost/numeric/ublas/matrix.hpp>
              > > #include <boost/numeric/ublas/io.hpp>
              > >
              > > int main () {
              > > using namespace boost::numeric::ublas;
              > > matrix<double> m (4, 4);
              > > for (unsigned int i = 0; i < m.size1 (); ++ i)
              > > for (unsigned int j = 0; j < m.size2 (); ++ j)
              > > m(i, j) = 4*i+j;
              > >
              > > std::cout << m << std::endl;
              > >
              > > swap(row(m,0), row(m,2));
              > > row(m,0).swap(row(m,2));
              > > swap(m.row(0), m.row(2));
              > > m.row(0).swap(m.row(2));
              > >
              > > matrix_row<matrix<double> > r1(m, 0), r2(m, 2);
              > >
              > > swap(r1, r2);
              > > r1.swap(r2);
              > >
              > > std::cout << m << std::endl;
              > > }
              >
              >
              > Compiles with VC++ 7, boost 1_30, too. I don't think that value parameters
              make a run
              > time difference here, sizeof( matrix_row) == 8

              Good observation.

              Thanks,

              Joerg
            • Julius Muschaweck
              Hi Michael, ... I took a look at the disassembly in VC 7 s debugger both in debug mode and release mode, for a similar case: struct Tmp { int i; int j; Tmp()
              Message 6 of 19 , Apr 9, 2003
              • 0 Attachment
                Hi Michael,

                At 19:51 08.04.2003, you wrote:
                Hi Joerg, Hi Kresimir,

                Kresimir Fresl wrote:

                >>Is there not an efficiency issue with the additional copy constructors
                >>necessary for the pass by value?
                >>Or are you assuming (hopeing) the compiler will completely optimise
                >>these away.
                >>If not it seems a high price to pay for the swap operation.
                >>   
                >>
                >
                >matrix_row<> object contains only a reference to the matrix
                >and a row index, so copy constructor is not very expensive.
                >
                and

                >Instead of copying pointer (which reference conceptually is),
                >pointer
                >
                >(Maybe smart compiler can optimize away double reference;
                >I don't know -- one should make some benchmarks.)

                >
                Since swap is usually assumed to be a fast basic operation even a small
                overhead associate with the copy constructor would be a very bad thing.
                Certianly compilers like gcc2.95 and VC6/7 will no optimise this away.
                I  took a look at the disassembly in VC 7's debugger both in debug mode and release mode, for a similar case:

                struct Tmp
                         {
                        
                int i; int j;
                         Tmp() : i(1), j(1) {};
                         Tmp(
                const Tmp& rhs ) : i(rhs.i), j(rhs.j) {};
                         Tmp&
                operator= (const Tmp& rhs) {i = rhs.i; j= rhs.j; return *this;};
                         };

                Tmp tmp( Tmp t )
                         {
                         t.i=42;
                        
                return t;
                         }

                Tmp tmpref( Tmp& t )
                         {
                         t.i=42;
                        
                return t;
                         }

                The copy constructor and assignment operator is duplicated to make Tmp a non-POD-class, like matrix_row.

                And indeed, in Debug mode there are all the calls to to copy constructors and so on.
                However, in Release mode, much is optimized away, as you can see below. There are a few more mov's, but no function calls, and these mov's should be inside fast cache. On the other hand, there is another indirection inside tmpref which balances the addtl mov's to some extent. So my conclusion is that yes, we should be very very careful not to introduce overhead into swap, but in this case here I don't expect to see a performance hit.

                         tt2 = tmp(tt);
                00405F41  sub         esp,8
                00405F44  mov         edi,1
                00405F49  mov         eax,esp
                00405F4B  mov         dword ptr [esp+18h],esp
                00405F4F  mov         dword ptr [eax],edi
                00405F51  mov         dword ptr [eax+4],edi
                00405F54  lea         eax,[esp+18h]
                00405F58  push        eax 
                00405F59  mov         dword ptr [esp+28h],edi
                00405F5D  mov         dword ptr [esp+24h],edi
                00405F61  call        tmp (401150h)
                00405F66  mov         ecx,dword ptr [eax]
                00405F68  mov         eax,dword ptr [eax+4]
                00405F6B  add         esp,0Ch
                         cout << tt2.i << ' ' << tt2.j << endl;
                [snip, this line included to force the compiler to actually look at tt2
                         tt2 = tmpref(tt);
                00405F9E  lea         ecx,[esp+18h]
                00405FA2  push        ecx 
                00405FA3  lea         edx,[esp+14h]
                00405FA7  push        edx 
                00405FA8  call        tmpref (401170h)
                00405FAD  mov         ecx,dword ptr [eax]
                00405FAF  mov         eax,dword ptr [eax+4]
                00405FB2  add         esp,8
                         cout << tt2.i << ' ' << tt2.j << endl;
                [snip, this line included to force the compiler to actually look at tt2

                But there is another thing to look at: The Standard (23.1) requires that swap() member functions of standard containers don't throw themselves. But matrix_row' s swap does throw bad_size() for a mismatch. And it has good reason to: swapping matrix rows of different sizes is a bad thing to do.
                So this would be one more reason to get rid of swap for matrix_rows and other ublas proxies. Since a proxy is an object in its own right but should be used as a reference only, we will always have confusion about swap(). The Standard's iter_swap function is in my mind indeed good guidance how a solution should look like. iter_swap gets it's iterators passed by value. And iter_swap isn't required not to throw.
                But proxies aren't iterators.
                 
                How about

                void swap_content( matrix_row<..> lhs, matrix_row<..> rhs)

                and the like for other proxies ?


                Julius

                Julius Muschaweck

                _____________________________________________________
                OEC AG
                Paul-Gerhardt-Allee 42
                81245 Muenchen, Germany

                Phone: +49 89 820050-30
                Fax:   +49 89 820050-41
                e-mail:    <muschaweck@...>
                Internet:   www.oec.net
                ______________________________________________________
              • jhr.walter@t-online.de
                Hi Julius, you wrote: [snip] ... and release ... *this;}; ... non-POD- ... and so on. ... There are a ... fast cache. On the other hand, there is another
                Message 7 of 19 , Apr 17, 2003
                • 0 Attachment
                  Hi Julius,

                  you wrote:

                  [snip]

                  > I took a look at the disassembly in VC 7's debugger both in debug mode
                  and release
                  > mode, for a similar case:
                  >
                  > struct Tmp
                  > {
                  > int i; int j;
                  > Tmp() : i(1), j(1) {};
                  > Tmp( const Tmp& rhs ) : i(rhs.i), j(rhs.j) {};
                  > Tmp& operator= (const Tmp& rhs) {i = rhs.i; j= rhs.j; return
                  *this;};
                  > };
                  >
                  > Tmp tmp( Tmp t )
                  > {
                  > t.i=42;
                  > return t;
                  > }
                  >
                  > Tmp tmpref( Tmp& t )
                  > {
                  > t.i=42;
                  > return t;
                  > }
                  >
                  > The copy constructor and assignment operator is duplicated to make Tmp a
                  non-POD-
                  > class, like matrix_row.
                  >
                  > And indeed, in Debug mode there are all the calls to to copy constructors
                  and so on.
                  > However, in Release mode, much is optimized away, as you can see below.
                  There are a
                  > few more mov's, but no function calls, and these mov's should be inside
                  fast cache. On the > other hand, there is another indirection inside tmpref
                  which balances the addtl mov's to
                  > some extent. So my conclusion is that yes, we should be very very careful
                  not to introduce > overhead into swap, but in this case here I don't expect
                  to see a performance hit.

                  Thanks for taking the time to investigate this. As far as I've learned in
                  the meantime, the standard library's binders have a similar problem. It
                  seems to be recommended practice to pass them by value due to language
                  idiosyncrasies.

                  [snip assembly]

                  > But there is another thing to look at: The Standard (23.1) requires that
                  swap() member
                  > functions of standard containers don't throw themselves. But matrix_row' s
                  swap does
                  > throw bad_size() for a mismatch.

                  (It throws or asserts depending from the environment). You have a point. Non
                  throwing swap() is essential for writing exception safe code AFAIK.

                  > And it has good reason to: swapping matrix rows of
                  > different sizes is a bad thing to do.
                  > So this would be one more reason to get rid of swap for matrix_rows and
                  other ublas
                  > proxies. Since a proxy is an object in its own right but should be used as
                  a reference only, > we will always have confusion about swap(). The
                  Standard's iter_swap function is in my
                  > mind indeed good guidance how a solution should look like. iter_swap gets
                  it's iterators
                  > passed by value. And iter_swap isn't required not to throw.
                  > But proxies aren't iterators.
                  >
                  > How about
                  >
                  > void swap_content( matrix_row<..> lhs, matrix_row<..> rhs)
                  >
                  > and the like for other proxies ?

                  OK, the original idea (requirement?) was, that one should be able to use
                  proxies or containers in generic algorithms transparently. We have relaxed
                  the corresponding requirements for the assignment operators of containers
                  recently to increase the standard library compatability. A change like the
                  above would break the corresponding interfaces. So I'm not sure, if it's a
                  good idea.

                  Best,

                  Joerg
                Your message has been successfully submitted and would be delivered to recipients shortly.