Move building of FooMesh to factory functions

[Martin Sandve Alnæs]

In this issue: https://bitbucket.org/fenics-project/dolfin/issue/66#comment-4881673

Garth wrote:

Having UnitSquareMesh, etc, as a subclass of Mesh is pain - it complicates code on the C++ side in cases too. The only difference between a Mesh and a UnitSquareMesh is the constructor. A better design would be to have mesh builder/mesh factory functions to build and return a unit foo Mesh.

Johan wrote:

It is nice to be able to write:

UnitSquareMesh mesh(10,10);

in C++, but the implementation could be done in a factory function, which is called in the constructor of FooMesh. In Python we could then ignore the classes and just call the factory function. Maybe also rename the factory function to FooMesh so user code wont break.

[Martin Sandve Alnæs]

To have factory functions but preserve:

UnitSquareMesh mesh(10,10);

would look something like this:

class UnitSquareMesh(Mesh)
{
    UnitSquareMesh(...) { build_unit_square_mesh(*this); }
};

In python we could do this instead:

def UnitSquareMesh(*args):
    mesh = Mesh()
    build_unit_square_mesh(mesh)
    return mesh

[Prof Garth Wells]

Or something like this:

Mesh mesh = MeshFactory::unit_square(10, 10);

and

def UnitSquareMesh(n0, n1):
    return MeshFactory::unit_square(n0, n1)

[Nico Schlömer]

I lean towards @garth-wells 's position here. Having Unit*Mesh classes seems overkill since the only thing we need is a function to produce a mesh which happens to live on a square, circle, etc.. Instead of a bunch of classes, it's easier to maintain a bunch of functions (or, putting them all in one entity, a factory).

[Anders Logg (Chalmers)]

Agree these should be factory functions but insist that we preserve the old class-like interface (possibly via a function named UnitSquareMesh).

With Garth's solution, I'm worried about the extra cost of assignment (in C++). Does the following function involve extra copying?

Mesh UnitSquareMesh { return MeshFactory::create_unit_square_mesh(8, 8); }

?

With Martin's solution, what is the advantage to the current design? UnitSquareMesh would still be a subclass of Mesh?

[Prof Garth Wells]

Most (all modern?) compilers use return value optimization to avoid copying. We have tested in the past for some objects, and gcc did not make a copy.

[Anders Logg (Chalmers)]

Yes, I know we have tested it many times in the past but I still get worried.

[Prof Garth Wells]

Even if a mesh is copied, it won't have any detectable effect on performance because (i) it is performed very few times in a program (most often only once) and (ii) the cost of copying is trivial compared to the work that is done in building the mesh.

[Prof Garth Wells]

See https://www.mail-archive.com/[e-mail]/msg01619.html for related discussion.

[Prof Garth Wells]

A good solution to this could be to add a static function toUnitFooMesh that returns a shared_ptr<Mesh>

static std::shared_ptr<Mesh> UnitFooMesh::create(.....);

What are the pros and cons of this versus

static UnitFooMesh::create(Mesh& mesh);

?

[Martin Sandve Alnæs]

The factory version disallows users calling create on an existing mesh (which is a good thing).

[Anders Logg (Chalmers)]

I like the latter one better:

static UnitFooMesh::create(Mesh& mesh);

It works with both references and pointers:

UnitFooMesh::create(mesh);
UnitFooMesh::create(*mesh);

I assume the new constructor will look the same as before and just call create?

[Prof Garth Wells]

I don't think the latter will work with smart pointers.

We could provide both interfaces. The factory approach makes it easier to get a shared pointer, and it maps better into Python.

[Chris Richardson]

One of the objectives of this issue is to allow parameters to be set on a Mesh before it is built (e.g. ghosting of cells in parallel). Does anyone have a good idea how this would look?

Mesh mesh;
mesh.parameters['ghost_layers'] = 3;
static UnitFooMesh::create(Mesh& mesh);

for example?

[Prof Garth Wells]

There should always be a function interface; parameters are for convenience, e.g. passing parameters from the command line.

[Chris Richardson]

What I am keen to avoid is this:

mesh = MeshFactory::UnitFooxMesh(20, 30, true, true, false);

How about:

mesh = MeshFactory::UnitFooxMesh(20, 30, parameters);

[Prof Garth Wells]

Use enums:

mesh0 = MeshFactory::create_unit_square(20, 30);
mesh1 = MeshFactory::create_unit_square(20, 30, MeshFactory::ghosted);

[Chris Richardson]

What some libraries do is to use enums which can be ored (or added) together,

mesh = MeshFactory::create_unit_square(20, 30, MeshFactory::ghosted | MeshFactory::leftright | MeshFactory::distributed);

which cuts down on the number of arguments...

[Prof Garth Wells]

Looks good. I've seen the syntax before, but don't know what the C++ implementation looks like. How would the interface from Python look?

[Johan Hake]

Same same.

ghosted = 1
leftright = 2
distributed = 4
print ghosted|leftright
print ghosted|leftright|distributed
print leftright|distributed

[Anders Logg (Chalmers)]

Just saying again that I support moving the implementation itself to factory functions, but still providing the wrapper classes to enable

UnitSquareMesh mesh(64, 64); // C++ mesh = UnitSquareMesh(64, 64) # Python

This notation is some of the most canonical/basic syntax we have in our API and has been there for a long long time. It looks good and people are used to it. The extra burden of adding the ~5 lines of code for each of the wrapper classes is small, considering we have a very limited number of builtin meshes.

[Martin Sandve Alnæs]

Anyone want to do this? It's a blocker for making dolfin Mesh a subclass ufl Mesh.

site-packages/dolfin/mesh/meshes.py:

# Replacement constructor for Mesh class. We cannot create a Python
# subclass of cpp.Mesh since then isinstance(UnitSquare, Mesh) would
# not be true.

[Martin Sandve Alnæs]

Note that the issue referenced at the top here is 'closed' but that's inaccurate. The integration of dolfin and ufl Mesh is not done and it's a critical step towards a number of features.

[Prof Garth Wells]

I think the 'fix' part is easy. Keeping backward compatibility will be a pain.

[Chris Richardson]

@martinal - I can start doing it. I've been holding off because the exact interface seemed a bit unclear, but I guess I should just do it, and then we can iterate on details...

[Martin Sandve Alnæs]

That would be great.

In the meantime, how important is the backwards compatibility of 'isinstance(unitsquare, Mesh)' really?

There's always 'isinstance(unitsquare, cpp.Mesh)' which should be sufficient for internal code.

@garth-wells @logg @blechta @augustjohansson @oyvinev @mikael_mortensen opinions on this particular question?

[Anders Logg (Chalmers)]

Removing milestone: 1.7 (automated comment)

[Jan Blechta]

@martinal, is the inheritance issue you mentioned obsolete?

>>> mesh = UnitSquareMesh(3, 3)
>>> isinstance(mesh, UnitSquareMesh)
True
>>> isinstance(mesh, Mesh)
True
>>> isinstance(mesh, cpp.Mesh)
True

Or do I understand you wrong?

[Prof Garth Wells]

Fixed in 4c55c05.