Should Python interface do range checking in release mode?

Issue #5 resolved
Prof Garth Wells created an issue

The below code gives an out-of-range error if DOLFIN is compiled with the DEBUG message, but returns rubbish otherwise. The question is should we have some range checking in the Python interface when compiled in release mode?

from dolfin import * m1 = UnitSquare(2, 2) m2 = UnitSquare(4, 4) cf = CellFunction("size_t", m1) c = Cell(m2, 0)

for i in range(20): print cf[i]

Comments (8)

  1. Johan Hake

    I would prefer having range check on any getfoo methods in Python. I can add it to the MeshFunction getfoo. I think it is there for most other cases.

  2. Jack Hale

    My personal opinion is absolutely yes. I've missed nasty bugs due to the lack of range checking in the Mesh class at the Python level. As far as I'm aware every other Python library which interfaces through to a C++ library they all perform range checking. The Python way is to hold your hand a little whilst coding. If I want performance I can write for the C++ library and in that environment I'm aware of the potential pitfalls.

    Also, is it possible to throw an error message if you use a MeshEntity iterator that has reached its end? See the following ipython log:

    In [1]: from dolfin import *
    
    In [2]: mesh = UnitSquareMesh(2,2)
    
    In [3]: for cell in cells(mesh):
       ...:     print cell
       ...:     
    <Mesh entity 0 of topological dimension 2>
    <Mesh entity 1 of topological dimension 2>
    <Mesh entity 2 of topological dimension 2>
    <Mesh entity 3 of topological dimension 2>
    <Mesh entity 4 of topological dimension 2>
    <Mesh entity 5 of topological dimension 2>
    <Mesh entity 6 of topological dimension 2>
    <Mesh entity 7 of topological dimension 2>
    
    In [4]: print cell
    <Mesh entity 4465036720 of topological dimension 4457365508>
    

    cell is now pointing to some random memory it seems, but it doesn't complain. Obviously the code is 'wrong' but I expect Python to help me out here.

  3. Martin Sandve Alnæs

    Definitely range checking in python. That is the expected behaviour for all python libraries.

    Jack: which version are you using? I get cell 7 repeated, but that could be random luck or somebody fixed this.

  4. Jack Hale

    Martin:

    Dolfin 1.2.0 compiled with dorsal using gcc 4.7 with MacPorts.

    The behaviour seems undetermined on my system, I actually just managed to get <Mesh entity 0 of topological dimension 0> to pop up running the same code.

    Perhaps this gives more information:

    In [3]: from dolfin import *
    
    In [4]: mesh = UnitSquareMesh(2,2)
    
    In [5]: for cell in cells(mesh):
       ...:     print cell.__repr__()
       ...:     
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cf00> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cfc0> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cf00> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cfc0> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cf00> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cfc0> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cf00> >
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cfc0> >
    
    In [6]: print cell.__repr__()
    <dolfin.cpp.mesh.Cell; proxy of <Swig Object of type 'dolfin::Cell *' at 0x11895cfc0> >
    
    In [7]: print cell
    <Mesh entity 7598258804607688714 of topological dimension 4190995773077137523>
    

    So I think I have a swig object pointing to the same underlying C++ object returned by the iterator. But it seems like that Cell object now points to locations in memory that contain junk values.

  5. Johan Hake

    Your problem was fixed in commit:

    19df8a3063ab9cce97cf97cd2b3bd1da7df56673

    The dereference call now return a cell by value instead of by reference.

    I have also added range check for the wrappers of getitem in MeshFunction, fixing this particular issue. The other getitem wrappers we have do check for range.

  6. Log in to comment