Move building of FooMesh to factory functions
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.
Comments (32)
-
reporter -
Or something like this:
Mesh mesh = MeshFactory::unit_square(10, 10);
and
def UnitSquareMesh(n0, n1): return MeshFactory::unit_square(n0, n1)
-
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). -
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?
-
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.
-
Yes, I know we have tested it many times in the past but I still get worried.
-
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.
-
See https://www.mail-archive.com/fenics@fenicsproject.org/msg01619.html for related discussion.
-
-
A good solution to this could be to add a static function to
UnitFooMesh
that returns ashared_ptr<Mesh>
static std::shared_ptr<Mesh> UnitFooMesh::create(.....);
What are the pros and cons of this versus
static UnitFooMesh::create(Mesh& mesh);
?
-
reporter The factory version disallows users calling create on an existing mesh (which is a good thing).
-
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?
-
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.
-
- changed milestone to 1.6
-
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?
-
There should always be a function interface; parameters are for convenience, e.g. passing parameters from the command line.
-
What I am keen to avoid is this:
mesh = MeshFactory::UnitFooxMesh(20, 30, true, true, false);
How about:
mesh = MeshFactory::UnitFooxMesh(20, 30, parameters);
-
Use enums:
mesh0 = MeshFactory::create_unit_square(20, 30); mesh1 = MeshFactory::create_unit_square(20, 30, MeshFactory::ghosted);
-
What some libraries do is to use enums which can be
or
ed (or added) together,mesh = MeshFactory::create_unit_square(20, 30, MeshFactory::ghosted | MeshFactory::leftright | MeshFactory::distributed);
which cuts down on the number of arguments...
-
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?
-
Same same.
ghosted = 1 leftright = 2 distributed = 4 print ghosted|leftright print ghosted|leftright|distributed print leftright|distributed
-
reporter - changed component to geometry
-
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.
-
reporter - changed milestone to 1.7
-
reporter 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.
-
reporter 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.
-
I think the 'fix' part is easy. Keeping backward compatibility will be a pain.
-
@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...
-
reporter 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?
-
- removed milestone
Removing milestone: 1.7 (automated comment)
-
@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?
-
- edited description
- changed status to resolved
Fixed in 4c55c05.
- Log in to comment
To have factory functions but preserve:
would look something like this:
In python we could do this instead: