Discussion:
[Pyublas] Cleanup up patch for iterators
Neal Becker
2009-02-02 12:20:17 UTC
Permalink
This version unifies const/non-const iterators.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: numpy.diff
Type: text/x-patch
Size: 9688 bytes
Desc: not available
URL: <http://lists.tiker.net/pipermail/pyublas/attachments/20090202/eaa2a14e/attachment.bin>
Andreas Klöckner
2009-02-09 06:06:43 UTC
Permalink
Hey Neal,
Post by Neal Becker
This version unifies const/non-const iterators.
Applied.

What's the point of

boost::mpl::if_<boost::is_const<T>,
const T, T>::type

? Looks like a no-op to me, I took it out. Let me know if that's wrong.

Also, it turns out that you have to emulate the iterator_facade example in the
documentation very closely. As much as I liked your solution with the vector
as a constructor argument, that breaks as soon a const begin is called,
because then the vector is const and doesn't fit the constructor argument any
more. These changes are here:
http://git.tiker.net/pyublas.git/commitdiff/43ae93

As far as the correctness of using
boost::numeric::ublas::dense_random_access_iterator_tag
is concerned, it does make norm_2 work and doesn't appear to hurt otherwise,
so let's leave it in.

Thanks for the patch.

Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.tiker.net/pipermail/pyublas/attachments/20090209/80683f19/attachment.pgp>
Neal Becker
2009-02-09 11:39:34 UTC
Permalink
Post by Andreas Klöckner
Hey Neal,
Post by Neal Becker
This version unifies const/non-const iterators.
Applied.
Sorry, I spoke too soon. I have reverted this because it broke some of my
code. It would be nice to have a unified version, but ATM I'm not sure how to
get it right, and I don't think it's worth the bother right now.
Post by Andreas Klöckner
What's the point of
boost::mpl::if_<boost::is_const<T>,
const T, T>::type
? Looks like a no-op to me, I took it out. Let me know if that's wrong.
Also, it turns out that you have to emulate the iterator_facade example in
the documentation very closely. As much as I liked your solution with the
vector as a constructor argument, that breaks as soon a const begin is
called, because then the vector is const and doesn't fit the constructor
http://git.tiker.net/pyublas.git/commitdiff/43ae93
As far as the correctness of using
boost::numeric::ublas::dense_random_access_iterator_tag
is concerned, it does make norm_2 work and doesn't appear to hurt
otherwise, so let's leave it in.
Thanks for the patch.
Andreas
Andreas Klöckner
2009-02-09 13:05:40 UTC
Permalink
Post by Neal Becker
Post by Andreas Klöckner
Hey Neal,
Post by Neal Becker
This version unifies const/non-const iterators.
Applied.
Sorry, I spoke too soon. I have reverted this because it broke some of my
code. It would be nice to have a unified version, but ATM I'm not sure how
to get it right, and I don't think it's worth the bother right now.
What was the breakage? Can you try/have you tried the version that's in git
now?

Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.tiker.net/pipermail/pyublas/attachments/20090209/051b890d/attachment.pgp>
Neal Becker
2009-02-09 13:54:29 UTC
Permalink
Post by Andreas Klöckner
Post by Neal Becker
Post by Andreas Klöckner
Hey Neal,
Post by Neal Becker
This version unifies const/non-const iterators.
Applied.
Sorry, I spoke too soon. I have reverted this because it broke some of
my code. It would be nice to have a unified version, but ATM I'm not
sure how to get it right, and I don't think it's worth the bother right
now.
What was the breakage? Can you try/have you tried the version that's in git
now?
Andreas
Can't seem to reproduce it. The current version compiles all my code.

The
// typename boost::mpl::if_<boost::is_const<T>,
// const T,
// T
// >::type,

doesn't seem to be needed, just replace with T.
Andreas Klöckner
2009-02-09 14:12:09 UTC
Permalink
Post by Neal Becker
Post by Andreas Klöckner
What was the breakage? Can you try/have you tried the version that's in
git now?
Andreas
Can't seem to reproduce it. The current version compiles all my code.
Excellent. :)
Post by Neal Becker
The
// typename boost::mpl::if_<boost::is_const<T>,
// const T,
// T
// >::type,
doesn't seem to be needed, just replace with T.
Already done.

Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.tiker.net/pipermail/pyublas/attachments/20090209/6b6199d5/attachment.pgp>
Neal Becker
2009-02-09 14:25:28 UTC
Permalink
Post by Andreas Klöckner
Post by Neal Becker
Post by Andreas Klöckner
What was the breakage? Can you try/have you tried the version that's in
git now?
Andreas
Can't seem to reproduce it. The current version compiles all my code.
Excellent. :)
Post by Neal Becker
The
// typename boost::mpl::if_<boost::is_const<T>,
// const T,
// T
// >::type,
doesn't seem to be needed, just replace with T.
Already done.
Andreas
How's this?

-------------- next part --------------
diff --git a/src/cpp/pyublas/numpy.hpp b/src/cpp/pyublas/numpy.hpp
index 590a809..578a7cd 100644
--- a/src/cpp/pyublas/numpy.hpp
+++ b/src/cpp/pyublas/numpy.hpp
@@ -833,10 +833,11 @@ namespace pyublas
class numpy_strided_vec_iterator
: public boost::iterator_facade<
numpy_strided_vec_iterator<T>,
- typename boost::mpl::if_<boost::is_const<T>,
- const T,
- T
- >::type,
+ T,
+ // typename boost::mpl::if_<boost::is_const<T>,
+ // const T,
+ // T
+ // >::type,
boost::numeric::ublas::dense_random_access_iterator_tag>
{
public:
@@ -847,21 +848,13 @@ namespace pyublas

typedef typename self_t::difference_type difference_type;

- numpy_strided_vec_iterator()
- : it(0)
+ numpy_strided_vec_iterator()
+ : stride (0), it(0)
{ }

- numpy_strided_vec_iterator(T *it)
- : it(it)
- { }
- /*
- numpy_strided_vec_iterator(
- numpy_strided_vector<typename self_t::value_type> &_vec)
- : stride (_vec.stride()),
- it (_vec.stride() >= 0 ?
- _vec.m_vector.array().begin() : _vec.m_vector.array().end()-1)
- { }
- */
+ numpy_strided_vec_iterator(T *it, typename numpy_strided_vector<T>::stride_type _stride)
+ : stride (_stride), it(it)
+ {}

// private:

@@ -969,33 +962,33 @@ namespace pyublas
iterator begin()
{
if (stride() >= 0)
- return iterator(this->m_vector.array().begin());
+ return iterator(this->m_vector.array().begin(), stride());
else
- return iterator(this->m_vector.array().end()-1);
+ return iterator(this->m_vector.array().end()-1, stride());
}

iterator end()
{
if (stride() >= 0)
- return iterator(this->m_vector.array().end());
+ return iterator(this->m_vector.array().end(), stride());
else
- return iterator(this->m_vector.array().end() - 1 - this->size());
+ return iterator(this->m_vector.array().end() - 1 - this->size(), stride());
}

const_iterator begin() const
{
if (stride() >= 0)
- return const_iterator(this->m_vector.array().begin());
+ return const_iterator(this->m_vector.array().begin(), stride());
else
- return const_iterator(this->m_vector.array().end()-1);
+ return const_iterator(this->m_vector.array().end()-1, stride());
}

const_iterator end() const
{
if (stride() >= 0)
- return const_iterator(this->m_vector.array().end());
+ return const_iterator(this->m_vector.array().end(), stride());
else
- return const_iterator(this->m_vector.array().end() - 1 - this->size());
+ return const_iterator(this->m_vector.array().end() - 1 - this->size(), stride());
}
};
Andreas Klöckner
2009-02-09 15:00:13 UTC
Permalink
Post by Neal Becker
How's this?
Check git--I committed essentially the same patch a few minutes ago.

Andreas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.tiker.net/pipermail/pyublas/attachments/20090209/d3018174/attachment.pgp>
Neal Becker
2009-02-09 14:06:46 UTC
Permalink
Why did you remove:
numpy_strided_vec_iterator (numpy_strided_vector & _vec) :
stride (_vec.stride()),
it (_vec.stride() >= 0 ? _vec.m_vector.array().begin() :
_vec.m_vector.array().end()-1)
{}

This change totally breaks strided iterators - it never sets the stride!
Andreas Klöckner
2009-02-09 14:29:55 UTC
Permalink
Post by Neal Becker
stride (_vec.stride()),
_vec.m_vector.array().end()-1)
{}
This change totally breaks strided iterators - it never sets the stride!
Oops. Good point. Try now.

(As an actual answer to your question: Recall how I said that initializing the
iterator with the vector as an argument breaks 'begin() const'. It wasn't
technically necessary to remove this constructor, but since it's unused now, I
did remove it.)

Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.tiker.net/pipermail/pyublas/attachments/20090209/74d99bd2/attachment.pgp>
Loading...