Re: [cython-users] allocation/initialization on stack of C++ iterator doesn't compile
175 views
Skip to first unread message
Robert Bradshaw
Oct 22, 2012, 9:32:16 PM10/22/12
to cython...@googlegroups.com
On Mon, Oct 22, 2012 at 9:18 AM, Jon Guyer <gu...@nist.gov> wrote:
> I am attempting to wrap the Mesh classes in [http://libmesh.sourceforge.net/
> libMesh] with Cython. These classes define different iterators to
> sequentially treat different elements of the Mesh and these iterators are
> returned by accessor methods. I am unable to write an iteration loop in
> Cython because I cannot initialize any of these iterators on the stack.
>
> The actual definitions in libMesh are a morass of "Templated forwarding
> ctor" scattered across a half-dozen headers, but it seems to distill to
> this:
>
[snip]
> If I change the Cython to
> {{{
> #!python
> cdef A a = self.thisptr.my_A()
> }}}
> I get the somewhat more informative
> {{{
> accessor.pyx:14:15: C++ class must have a no-arg constructor to be stack
> allocated
> }}}
Which is the error we should be throwing.
> but that doesn't address the fact that the the equivalent initialization is legal C++.
The problem with the one-line solution is that it requires careful
control over the scope in which variables are declared vs.
initialized. Every bracket in C++ defines a new scope, which is not
the case in Python/Cython, hence the restriction (e.g. variables
assigned in a loop body can be used outside of it). Though it's simple
to make it work by manually tweaking the C in this case, the generic
case is much more complicated (and potentially ends up leaking the
scope constraints to the Python level).
Of course, this is a bug that should be fixed, but in the meantime
you'll have to allocate this as a pointer (which is what we'd have to
do under the hood to support this).
BTW, in Cython 0.17 you can iterate over C++ iterators using "for x in
myInstance: ..." which is much cleaner.
> More significantly, in the actual code, where `node_iterator` is declared as
> a subclass of `Mesh`
> {{{
> #!python
> cdef Mesh.node_iterator it = self.thisptr.active_nodes_begin()
> }}}
> results in
> {{{
> libMeshMesh.pyx:117:13: 'Mesh' is not a cimported module
> }}}
How did you declare Mesh?
- Robert
> I am attempting to wrap the Mesh classes in [http://libmesh.sourceforge.net/
> libMesh] with Cython. These classes define different iterators to
> sequentially treat different elements of the Mesh and these iterators are
> returned by accessor methods. I am unable to write an iteration loop in
> Cython because I cannot initialize any of these iterators on the stack.
>
> The actual definitions in libMesh are a morass of "Templated forwarding
> ctor" scattered across a half-dozen headers, but it seems to distill to
> this:
>
[snip]
> If I change the Cython to
> {{{
> #!python
> cdef A a = self.thisptr.my_A()
> }}}
> I get the somewhat more informative
> {{{
> accessor.pyx:14:15: C++ class must have a no-arg constructor to be stack
> allocated
> }}}
Which is the error we should be throwing.
> but that doesn't address the fact that the the equivalent initialization is legal C++.
The problem with the one-line solution is that it requires careful
control over the scope in which variables are declared vs.
initialized. Every bracket in C++ defines a new scope, which is not
the case in Python/Cython, hence the restriction (e.g. variables
assigned in a loop body can be used outside of it). Though it's simple
to make it work by manually tweaking the C in this case, the generic
case is much more complicated (and potentially ends up leaking the
scope constraints to the Python level).
Of course, this is a bug that should be fixed, but in the meantime
you'll have to allocate this as a pointer (which is what we'd have to
do under the hood to support this).
BTW, in Cython 0.17 you can iterate over C++ iterators using "for x in
myInstance: ..." which is much cleaner.
> More significantly, in the actual code, where `node_iterator` is declared as
> a subclass of `Mesh`
> {{{
> #!python
> cdef Mesh.node_iterator it = self.thisptr.active_nodes_begin()
> }}}
> results in
> {{{
> libMeshMesh.pyx:117:13: 'Mesh' is not a cimported module
> }}}
How did you declare Mesh?
- Robert
Jonathan Guyer
Oct 23, 2012, 8:33:06 AM10/23/12
to cython...@googlegroups.com
On Oct 22, 2012, at 9:32 PM, Robert Bradshaw wrote:
> On Mon, Oct 22, 2012 at 9:18 AM, Jon Guyer <gu...@nist.gov> wrote:
>> I am unable to write an iteration loop in
>> Cython because I cannot initialize any of these iterators on the stack.
>
>> but that doesn't address the fact that the the equivalent initialization is legal C++.
>
> The problem with the one-line solution is that it requires careful
> control over the scope in which variables are declared vs.
> initialized. Every bracket in C++ defines a new scope, which is not
> the case in Python/Cython, hence the restriction (e.g. variables
> assigned in a loop body can be used outside of it). Though it's simple
> to make it work by manually tweaking the C in this case, the generic
> case is much more complicated (and potentially ends up leaking the
> scope constraints to the Python level).
Thanks very much for your thorough explanation. I had not appreciated the broader issue you were tangling with. I guess my inclination would be to generate legal C++ code, limiting the scope of the variables from what Python would otherwise allow. This is a hybrid language after all and some constraints are inevitable. You see the implications of these choices much more clearly than I do, though.
>
> The problem with the one-line solution is that it requires careful
> control over the scope in which variables are declared vs.
> initialized. Every bracket in C++ defines a new scope, which is not
> the case in Python/Cython, hence the restriction (e.g. variables
> assigned in a loop body can be used outside of it). Though it's simple
> to make it work by manually tweaking the C in this case, the generic
> case is much more complicated (and potentially ends up leaking the
> scope constraints to the Python level).
> Of course, this is a bug that should be fixed, but in the meantime
> you'll have to allocate this as a pointer (which is what we'd have to
> do under the hood to support this).
{{{
cdef A *a_ptr = &self.thisptr.my_A()
}}}
and
{{{
cdef A *a_ptr
deref(a_ptr) = self.thisptr.my_A()
}}}
neither of which I really expected to work, and which I knew were dangerous constructs in C++ if they did.
I now see that
{{{
cdef A *a_ptr = new A(self.thisptr.my_A())
}}}
seems to do the trick.
Unfortunately, I'm then stymied by the "'Mesh' is not a cimported module" issue below when I try to do it for real.
> BTW, in Cython 0.17 you can iterate over C++ iterators using "for x in
> myInstance: ..." which is much cleaner.
I initially tried
{{{
for it in self.thisptr.active_nodes_begin():
}}}
but that raises
{{{
libMeshMesh.pyx:121:18: missing begin() on node_iterator
}}}
I then realized that you said myInstance and not myIterator, but the problem is that a libMesh Mesh has multiple iterators over both its nodes and its elements:
{{{
virtual element_iterator elements_begin () = 0;
virtual element_iterator elements_end () = 0;
virtual element_iterator active_elements_begin () = 0;
virtual element_iterator active_elements_end () = 0;
virtual element_iterator ancestor_elements_begin () = 0;
virtual element_iterator ancestor_elements_end () = 0;
virtual element_iterator subactive_elements_begin () = 0;
virtual element_iterator subactive_elements_end () = 0;
virtual element_iterator not_active_elements_begin () = 0;
virtual element_iterator not_active_elements_end () = 0;
virtual element_iterator not_ancestor_elements_begin () = 0;
virtual element_iterator not_ancestor_elements_end () = 0;
:
:
virtual node_iterator nodes_begin () = 0;
virtual node_iterator nodes_end () = 0;
virtual node_iterator active_nodes_begin () = 0;
virtual node_iterator active_nodes_end () = 0;
virtual node_iterator local_nodes_begin () = 0;
virtual node_iterator local_nodes_end () = 0;
virtual node_iterator pid_nodes_begin (const unsigned int proc_id) = 0;
virtual node_iterator pid_nodes_end (const unsigned int proc_id) = 0;
}}}
A Mesh has no explicit begin() and end() and they would be completely ambiguous if it did. I realize that the semantics might not be quite right, but for this use case, I'd really like to be able to write:
{{{
for it in self.thisptr.active_nodes_begin():
}}}
>> More significantly, in the actual code, where `node_iterator` is declared as
>> a subclass of `Mesh`
>> {{{
>> #!python
>> cdef Mesh.node_iterator it = self.thisptr.active_nodes_begin()
>> }}}
>> results in
>> {{{
>> libMeshMesh.pyx:117:13: 'Mesh' is not a cimported module
>> }}}
>
> How did you declare Mesh?
{{{
>> More significantly, in the actual code, where `node_iterator` is declared as
>> a subclass of `Mesh`
>> {{{
>> #!python
>> cdef Mesh.node_iterator it = self.thisptr.active_nodes_begin()
>> }}}
>> results in
>> {{{
>> libMeshMesh.pyx:117:13: 'Mesh' is not a cimported module
>> }}}
>
> How did you declare Mesh?
cdef extern from "node.h" namespace "libMesh":
cdef cppclass Node:
Node()
cdef extern from "mesh.h" namespace "libMesh":
cdef cppclass Mesh:
cppclass node_iterator:
Node* operator*()
node_iterator operator++()
bint operator==(node_iterator)
bint operator!=(node_iterator)
Mesh(unsigned int dim)
node_iterator active_nodes_begin()
node_iterator active_nodes_end()
}}}
Robert Bradshaw
Oct 23, 2012, 1:19:18 PM10/23/12
to cython...@googlegroups.com
On Tue, Oct 23, 2012 at 5:33 AM, Jonathan Guyer <gu...@nist.gov> wrote:
>
> On Oct 22, 2012, at 9:32 PM, Robert Bradshaw wrote:
>
>> On Mon, Oct 22, 2012 at 9:18 AM, Jon Guyer <gu...@nist.gov> wrote:
>>> I am unable to write an iteration loop in
>>> Cython because I cannot initialize any of these iterators on the stack.
>>
>>> but that doesn't address the fact that the the equivalent initialization is legal C++.
>>
>> The problem with the one-line solution is that it requires careful
>> control over the scope in which variables are declared vs.
>> initialized. Every bracket in C++ defines a new scope, which is not
>> the case in Python/Cython, hence the restriction (e.g. variables
>> assigned in a loop body can be used outside of it). Though it's simple
>> to make it work by manually tweaking the C in this case, the generic
>> case is much more complicated (and potentially ends up leaking the
>> scope constraints to the Python level).
>
> Thanks very much for your thorough explanation. I had not appreciated the broader issue you were tangling with. I guess my inclination would be to generate legal C++ code, limiting the scope of the variables from what Python would otherwise allow. This is a hybrid language after all and some constraints are inevitable. You see the implications of these choices much more clearly than I do, though.
We certainly shouldn't be generating invalid C++ code. As for limiting
>
> On Oct 22, 2012, at 9:32 PM, Robert Bradshaw wrote:
>
>> On Mon, Oct 22, 2012 at 9:18 AM, Jon Guyer <gu...@nist.gov> wrote:
>>> I am unable to write an iteration loop in
>>> Cython because I cannot initialize any of these iterators on the stack.
>>
>>> but that doesn't address the fact that the the equivalent initialization is legal C++.
>>
>> The problem with the one-line solution is that it requires careful
>> control over the scope in which variables are declared vs.
>> initialized. Every bracket in C++ defines a new scope, which is not
>> the case in Python/Cython, hence the restriction (e.g. variables
>> assigned in a loop body can be used outside of it). Though it's simple
>> to make it work by manually tweaking the C in this case, the generic
>> case is much more complicated (and potentially ends up leaking the
>> scope constraints to the Python level).
>
> Thanks very much for your thorough explanation. I had not appreciated the broader issue you were tangling with. I guess my inclination would be to generate legal C++ code, limiting the scope of the variables from what Python would otherwise allow. This is a hybrid language after all and some constraints are inevitable. You see the implications of these choices much more clearly than I do, though.
the scope, we try to follow Python semantics as close as possible, and
it would likely be confusing for the scope to ill-defined based on the
underlying implementation (which a user only has indirect control
over).
Likely what we'll eventually do is fake stack allocation.
>> Of course, this is a bug that should be fixed, but in the meantime
>> you'll have to allocate this as a pointer (which is what we'd have to
>> do under the hood to support this).
>
> OK, I had tried that, but wasn't getting it right. libMesh only returns its iterators by value and I was having trouble figuring out how to get a pointer out of that. I had tried to do things like
> {{{
> cdef A *a_ptr = &self.thisptr.my_A()
> }}}
> and
> {{{
> cdef A *a_ptr
> deref(a_ptr) = self.thisptr.my_A()
> }}}
> neither of which I really expected to work, and which I knew were dangerous constructs in C++ if they did.
>
> I now see that
> {{{
> cdef A *a_ptr = new A(self.thisptr.my_A())
> }}}
> seems to do the trick.
>
> Unfortunately, I'm then stymied by the "'Mesh' is not a cimported module" issue below when I try to do it for real.
we could resurrect the for...from syntax for this:
for it from self.thisptr.active_nodes_begin() <= it <
self.thisptr.active_noces_end():
...
Another solution is to create a simple wrapper class ActiveNodes whose
end() and begin() method invoke active_nodes_begin, so you could write
for item in ActiveNodes(self.thisptr):
...
>>> More significantly, in the actual code, where `node_iterator` is declared as
>>> a subclass of `Mesh`
>>> {{{
>>> #!python
>>> cdef Mesh.node_iterator it = self.thisptr.active_nodes_begin()
>>> }}}
>>> results in
>>> {{{
>>> libMeshMesh.pyx:117:13: 'Mesh' is not a cimported module
>>> }}}
>>
>> How did you declare Mesh?
>
> {{{
> cdef extern from "node.h" namespace "libMesh":
> cdef cppclass Node:
> Node()
>
> cdef extern from "mesh.h" namespace "libMesh":
> cdef cppclass Mesh:
> cppclass node_iterator:
> Node* operator*()
> node_iterator operator++()
> bint operator==(node_iterator)
> bint operator!=(node_iterator)
> Mesh(unsigned int dim)
> node_iterator active_nodes_begin()
> node_iterator active_nodes_end()
> }}}
>
template classes (e.g. vector) so I guess this regression was never
detected. I sense a bugfix release coming up...
- Robert
Jonathan Guyer
Oct 25, 2012, 9:47:45 AM10/25/12
to cython...@googlegroups.com
On Oct 23, 2012, at 1:19 PM, Robert Bradshaw wrote:
> Another solution is to create a simple wrapper class ActiveNodes whose
> end() and begin() method invoke active_nodes_begin, so you could write
>
> for item in ActiveNodes(self.thisptr):
> ...
== mesh_iterator.h ==
{{{
#!cpp
#import "mesh.h"
namespace libMesh {
typedef Mesh::node_iterator node_iterator;
class ActiveNodes
{
public:
inline ActiveNodes(Mesh *meshptr) {
this->meshptr = meshptr;
}
inline node_iterator begin() {
return this->meshptr->active_nodes_begin();
}
inline node_iterator end() {
return this->meshptr->active_nodes_end();
}
private:
Mesh *meshptr;
};
}
}}}
== libMeshMesh.pxd ==
{{{
#!python
cdef extern from "mesh_iterators.h" namespace "libMesh":
cppclass node_iterator:
Node* operator*()
node_iterator operator++()
bint operator==(node_iterator)
bint operator!=(node_iterator)
cdef cppclass ActiveNodes:
Node* operator*()
node_iterator operator++()
bint operator==(node_iterator)
bint operator!=(node_iterator)
ActiveNodes(Mesh *meshptr)
node_iterator begin()
node_iterator end()
}}}
and
== libMeshMesh.pyx ==
{{{
#!python
cdef class PyMesh:
cdef Mesh *thisptr
def __cinit__(self, unsigned int dim=1):
self.thisptr = new Mesh(dim)
def print_nodes(self):
cdef Node *it
for it in ActiveNodes(self.thisptr):
pass
}}}
and g++ raises
{{{
/Users/guyer/Documents/research/FiPy/fipy/fipy/meshes/libMesh/libMeshMesh.cpp: In function ‘PyObject* __pyx_pf_11libMeshMesh_6PyMesh_8print_nodes(__pyx_obj_11libMeshMesh_PyMesh*)’:
/Users/guyer/Documents/research/FiPy/fipy/fipy/meshes/libMesh/libMeshMesh.cpp:2060: error: no matching function for call to ‘libMesh::MeshBase::node_iterator::node_iterator()’
/Users/guyer/Downloads/libmesh-0.7.3.2/libmesh/include/mesh/mesh_base.h:881: note: candidates are: libMesh::MeshBase::node_iterator::node_iterator(const libMesh::MeshBase::node_iterator&)
}}}
I'm back where I started because node_iterator doesn't have a no-arg constructor.
The generated code is:
== libMeshMesh.cpp ==
static PyObject *__pyx_pf_11libMeshMesh_6PyMesh_8print_nodes(struct __pyx_obj_11libMeshMesh_PyMesh *__pyx_v_self) {
CYTHON_UNUSED libMesh::Node *__pyx_v_it;
PyObject *__pyx_r = NULL;
__Pyx_RefNannyDeclarations
libMesh::node_iterator __pyx_t_1;
libMesh::ActiveNodes *__pyx_t_2;
libMesh::Node *__pyx_t_3;
__Pyx_RefNannySetupContext("print_nodes", 0);
/* "libMeshMesh.pyx":125
* # pass
* cdef Node *it
* for it in ActiveNodes(self.thisptr): # <<<<<<<<<<<<<<
* pass
*
*/
__pyx_t_2 = &libMesh::ActiveNodes(__pyx_v_self->thisptr);
__pyx_t_1 = __pyx_t_2->begin();
for (;;) {
if (!(__pyx_t_1 != __pyx_t_2->end())) break;
__pyx_t_3 = *__pyx_t_1;
++__pyx_t_1;
__pyx_v_it = __pyx_t_3;
}
__pyx_r = Py_None; __Pyx_INCREF(Py_None);
__Pyx_XGIVEREF(__pyx_r);
__Pyx_RefNannyFinishContext();
return __pyx_r;
}
where, in this case, it seems like the scope is unambiguous and it really could be
{{{
#!diff
2060d2059
< libMesh::node_iterator __pyx_t_1;
2073c2072
< __pyx_t_1 = __pyx_t_2->begin();
---
> libMesh::node_iterator __pyx_t_1 = __pyx_t_2->begin();
}}}
> Oh, that does look like a bug. All our tests of nested classes use
> template classes (e.g. vector) so I guess this regression was never
> detected. I sense a bugfix release coming up...
Looking forward to it.
> Oh, that does look like a bug. All our tests of nested classes use
> template classes (e.g. vector) so I guess this regression was never
> detected. I sense a bugfix release coming up...
Robert Bradshaw
Oct 30, 2012, 4:17:12 AM10/30/12
to cython...@googlegroups.com
>> Oh, that does look like a bug. All our tests of nested classes use
>> template classes (e.g. vector) so I guess this regression was never
>> detected. I sense a bugfix release coming up...
, Let's see if there's anything else that should go in.
- Robert
Robert Bradshaw
Oct 30, 2012, 4:25:40 AM10/30/12
to cython...@googlegroups.com
https://github.com/cython/cython/blob/0.17/Cython/Compiler/ExprNodes.py#L2069
Jonathan Guyer
Oct 31, 2012, 1:18:22 PM10/31/12
to cython...@googlegroups.com
On Oct 30, 2012, at 4:17 AM, Robert Bradshaw wrote:
>> it seems like the scope is unambiguous and it really could be
>>
>> {{{
>> #!diff
>> 2060d2059
>> < libMesh::node_iterator __pyx_t_1;
>> 2073c2072
>> < __pyx_t_1 = __pyx_t_2->begin();
>> ---
>>> libMesh::node_iterator __pyx_t_1 = __pyx_t_2->begin();
>> }}}
>
> Hmm... yes.
>
>>> Oh, that does look like a bug. All our tests of nested classes use
>>> template classes (e.g. vector) so I guess this regression was never
>>> detected. I sense a bugfix release coming up...
>
> Fix at https://github.com/cython/cython/commit/f9068f3de1ca260e0ff70e64c07b934adf9ef317
> , Let's see if there's anything else that should go in.
In the interim, I was able to work around the issue of not being able to refer to a nested class by cdef'ing `node_iterator` outside the cdef for `Mesh`:
== libMeshMesh.pxd ==
{{{
#!python
cdef extern from "mesh.h" namespace "libMesh::Mesh":
cdef cppclass node_iterator:
node_iterator(node_iterator&)
Node* operator*()
node_iterator operator++()
bint operator==(node_iterator)
bint operator!=(node_iterator)
}}}
and the issue of not having a no-arg constructor by doing
node_iterator operator++()
bint operator==(node_iterator)
bint operator!=(node_iterator)
}}}
== libMeshMesh.pyx ==
{{{
#!python
cdef class PyMesh:
cdef Mesh *thisptr
def __cinit__(self, unsigned int dim=1):
self.thisptr = new Mesh(dim)
def print_nodes(self):
while deref(it) != self.thisptr.active_nodes_end():
inc(deref(it))
}}}
Reply all
Reply to author
Forward
0 new messages