On 23 June 2015 at 11:00, Jan Blechta <[email protected]> wrote:
> On Tue, 23 Jun 2015 08:54:03 +0000 > Anders Logg <[email protected]> wrote: > > > Yes exactly, that's the point, but it would get rid of a few lines of > > code and look simpler. Users who want to write more lines of code are > > free to do this: > > > > BoundingBoxTree tree; > > tree.build(mesh); > > const std::size_t cell_index = > > tree.compute_first_entity_collision(x); // this may fail and give a > > non-context related error message Cell cell(mesh, cell_index); > > function.eval(values, x, cell); > > > > Compare with: > > > > AutoCell cell; // we don't even need to supply the mesh here > > function.eval(values, x, cell); > > Ok, I'm just saying that this is equivalent to > > BoundingBoxTree tree(mesh); > function.eval(values, x, tree); > > while not introducing new class. But if it creates some benefit that > AutoCell inherits from Cell, then why not... > > Jan > Because it's a strange abstraction and doesn't solve any problems that needs solving, but introduces new hidden pitfalls: AutoCell cell; // we don't even need to supply the mesh here function.eval(values, x, cell); // but the mesh is secretly attached here So now cell contains a big data structure (the tree) and a reference to the mesh, and the user still needs to carry the AutoCell object around to avoid to gain any benefits related to reuse of the tree. Why not just carry the tree around? With Jan's example, it's much clearer how the data flows: BoundingBoxTree tree(mesh); function.eval(values, x, tree); and now the user has a reusable tree in the same amount of code. Martin > > > -- > > Anders > > > > > > tis 23 juni 2015 kl 10:48 skrev Jan Blechta > > <[email protected]>: > > > > > On Tue, 23 Jun 2015 08:42:49 +0000 > > > Anders Logg <[email protected]> wrote: > > > > > > > How about something like this: > > > > > > > > 1. Require additional Cell& cell argument to Function::eval > > > > > > > > 2. Add new class AutoCell handling this for users who don't want > > > > to explicitly work with a BoundingBoxTree > > > > > > > > class AutoCell : public Cell > > > > { > > > > public: > > > > AutoCell(Mesh &mesh); > > > > BoundingBoxTree tree; > > > > } > > > > > > > > AutoCell cell(mesh); > > > > function.eval(values, x, cell); > > > > > > > > 3. If the mesh moves, a user can do something like: > > > > > > > > cell.invalidate() > > > > > > This is semantically equivalent to passing bounding box. AutoCell > > > would be just a wrapper for BoundingBoxTree, not doing anything new. > > > > > > Jan > > > > > > > > > > > -- > > > > Anders > > > > > > > > > > > > > > > > tis 23 juni 2015 kl 10:30 skrev Garth N. Wells <[email protected]>: > > > > > > > > > On 22 June 2015 at 18:00, Anders Logg <[email protected]> > > > > > wrote: > > > > > > The challenge in moving the bounding box tree outside of the > > > > > > mesh is > > > > > that it > > > > > > has always been part of the mesh (it replaced an earlier data > > > > > > structure > > > > > that > > > > > > was there) so a user expects to be able to do > > > > > > > > > > > > v = Function(V) > > > > > > print v(x) > > > > > > > > > > > > > > > > This is fine for Expressions, but for a Function I don't think > > > > > it's bad for the interface to make obvious to the user that > > > > > they are performing a potentially expensive operation. If the > > > > > user was required to pass the cell, all would be fine. It would > > > > > also fix the issues with Function evaluations in parallel. > > > > > > > > > > > without needing to instantiate some cryptic BoundingBoxTree > > > > > > data > > > > > structure. > > > > > > Furthermore, a user expects that on subsequent calls v(x) is > > > > > > fast since > > > > > the > > > > > > tree has already been built. > > > > > > > > > > > > I don't see a way around automatic handling of building the > > > > > > search tree. > > > > > Are > > > > > > there some clever suggestions? > > > > > > > > > > > > > > > > We have a fundamental problem/flaw that MeshGeometery is mutable > > > > > and a Mesh owns a bounding box object. One of the two needs to > > > > > give. > > > > > > > > > > Garth > > > > > > > > > > > > > > > > Handling this for PointSource and MultiMesh is unproblematic. > > > > > > (But for MultiMesh, I would rather want to move it myself.) > > > > > > > > > > > > -- > > > > > > Anders > > > > > > > > > > > > > > > > > > mån 22 juni 2015 kl 17:54 skrev Marco Morandini < > > > > > [email protected]>: > > > > > >> > > > > > >> >>> Besides, the mesh bounding_box_tree used to find the > > > > > >> >>> colliding mesh entity is cached. I fear this could be a > > > > > >> >>> source of "strange" results, because its use is here > > > > > >> >>> completely transparent to the user, who may be unaware of > > > > > >> >>> the need to update it. > > > > > >> >>> > > > > > >> >> > > > > > >> >> I really don't like magical caching. How about having > > > > > >> >> > > > > > >> >> PointSource::apply(GenericVector& b, Cell& c, double > > > > > >> >> magnitude); > > > > > >> >> > > > > > >> >> The user is responsible for finding the cell, and thereby > > > > > >> >> also responsible for handling meshes that move, etc. > > > > > >> >> > > > > > >> >> PointSource::apply(...) presently uses > > > > > >> >> Mesh::bounding_box_tree(), which I would like to get rid > > > > > >> >> of from Mesh since mesh geometry is mutable. If the > > > > > >> >> search tools are not cached, the user takes > > > > > >> >> responsibility for managing the bounding boxes. > > > > > >> > > > > > > >> > For the record, this issue is filed as > > > > > >> > https://bitbucket.org/fenics-project/dolfin/issue/89 > > > > > >> > > > > > > >> > > > > > >> Right now Mesh::bounding_box_tree() is used by > > > > > >> > > > > > >> void PointSource::apply(GenericVector& b) > > > > > >> void Function::eval(Array<double>& values, const > > > > > >> Array<double>& x) const > > > > > >> > > > > > >> and > > > > > >> > > > > > >> MultiMesh ( + MultiMeshDirichletBC ) > > > > > >> > > > > > >> It would be pretty easy to change PointSource and Function. > > > > > >> But I think that, for MultiMesh, I should move the bboxes > > > > > >> there, and leave MultiMesh::build() unchanged. > > > > > >> > > > > > >> I can go this route if there is consensus. > > > > > >> > > > > > >> Marco > > > > > >> > > > > > >> > > > > > >> > > > > > >> _______________________________________________ > > > > > >> fenics mailing list > > > > > >> [email protected] > > > > > >> http://fenicsproject.org/mailman/listinfo/fenics > > > > > _______________________________________________ > > > > > fenics mailing list > > > > > [email protected] > > > > > http://fenicsproject.org/mailman/listinfo/fenics > > > > > > > > > > > _______________________________________________ > > > fenics mailing list > > > [email protected] > > > http://fenicsproject.org/mailman/listinfo/fenics > > > > > _______________________________________________ > fenics mailing list > [email protected] > http://fenicsproject.org/mailman/listinfo/fenics >
_______________________________________________ fenics mailing list [email protected] http://fenicsproject.org/mailman/listinfo/fenics
