dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

On Sun, Oct 26, 2008 at 08:25:48PM +0000, Garth N. Wells wrote:
> 
> 
> Garth N. Wells wrote:
> > 
> > Anders Logg wrote:
> >> On Sun, Oct 26, 2008 at 07:49:13PM +0000, Garth N. Wells wrote:
> >>> Anders Logg wrote:
> >>>> On Sun, Oct 26, 2008 at 12:12:06PM +0000, Garth N. Wells wrote:
> >>>>> Anders Logg wrote:
> >>>>>> On Sun, Oct 26, 2008 at 12:50:12PM +0100, DOLFIN wrote:
> >>>>>>> One or more new changesets pushed to the primary dolfin repository.
> >>>>>>> A short summary of the last three changesets is included below.
> >>>>>>>
> >>>>>>> changeset:   5003:f2effd253de3ae72894bdecb768447b8336a5014
> >>>>>>> tag:         tip
> >>>>>>> user:        "Garth N. Wells <gnw20@xxxxxxxxx>"
> >>>>>>> date:        Sun Oct 26 11:50:01 2008 +0000
> >>>>>>> files:       dolfin/ale/HarmonicSmoothing.cpp dolfin/mf/MatrixFactory.cpp
> >>>>>>> description:
> >>>>>>> DOLFIN compiles again!
> >>>>>> Nice!
> >>>>>>
> >>>>> We need to have a good look at const in the mesh classes. I used quite a 
> >>>>> bit of cont_cast to get things worker because it's always easier to 
> >>>>> start with something that compiles (and we can continue with the 
> >>>>> Function interface if it compiles). A few things:
> >>>> Good. I agree it's best to get it to compile, then try removing all
> >>>> the const_casts.
> >>>>
> >>>>> - Do we want to be able to call functions like Mesh::order on const 
> >>>>> meshes? I would suggest yes because it's nice to make objects like Form 
> >>>>>   const because they are 95% const, but it is necessary to call various 
> >>>>> mesh functions. We would need to use mutable where necessary in the mesh 
> >>>>> classes.
> >>>> I think the init() functions can very well be const (and use
> >>>> const_cast internally). 
> >>> Isn't this when mutable comes in handy? It avoids the const_cast inside 
> >>> init().
> >> Yes, but then we would need to make MeshConnectivity mutable which is
> >> essentially all mesh data except for the coordinates, so then const
> >> has little meaning.
> >>
> > 
> > ok
> > 
> >>> One may say that for example the edges of a
> >>>> Mesh always exist, even though that may not have been computed. So,
> >>>> one should be able to do
> >>>>
> >>>>   const Mesh& mesh;
> >>>>   for (EdgeIterator edge(mesh); !edge.end(); ++edge)
> >>>>   {
> >>>>
> >>>>   }
> >>>>
> >>>> even if this may potentially lead to a precomputation of all edges at the
> >>>> start of the loop.
> >>>>
> >>>> However, I don't think it's a good idea to make Mesh::order() const,
> >>>> since it may potentially change the numbering of the mesh. So a user
> >>>> could then assemble over a mesh and as a result the mesh will have
> >>>> changed, even though the assemble() function promises to be const
> >>>> regarding the Mesh (Form).
> >>>>
> >>>> This is problematic considering we now automatically initialiaze
> >>>> edges, faces etc and make sure they are ordered. What I think we need
> >>>> to do is to check whether or not the mesh is ordered and if not so
> >>>> give an error message saying that the mesh has not been correctly
> >>>> ordered. In those cases, a user will need to do
> >>>>
> >>>>   mesh.order()
> >>>>
> >>>> and then call assemble().
> >>>>
> >>> I'm wouldn't be very happy with this solution because I it breaks the 
> >>> simple interface. It's not intuitive that a user should call mesh.order().
> >>>
> >>> No ideal, but what if we don't pass 'const Form' to Assemblerm but just 
> >>> 'Form' instead. We have a degree of control by adding only const member 
> >>> functions to Form with exceptions as needed.
> >> I think one should be able to expect that the Form is not changed when
> >> something is assembled from it.
> >>
> >> I've added a check in DofMap::init() instead of calling order(). Let's
> >> see if it causes any trouble. I think it's a good thing that we don't
> >> hide the reordering of the mesh. It may be very confusing to try to
> >> debug an application if the mesh may be silently reordered.
> >>
> >> We could possibly use a const_cast in DofMap::init() and issue a
> >> warning that the mesh has been renumbered.
> >>
> > 
> > Can you remind why we need to re-order? Is it because we don't require a 
> > connectivity order in the input mesh?
> > 
> > It is rare that it's needed, I don't mind an error message if 
> 
> Flip those first two words - "Is it rare . . . "
> 
> Garth
> 
> > mesh.order() is required for only special meshes (e.g., possible those 
> > not created by dolfin-convert)
> > 
> >>>>> - We should add cont version of the various mesh iterators.
> >>>> Do we need that? My idea with the mesh iterators (now) was that they
> >>>> are always const in the sense that they can be created from a const
> >>>> Mesh or MeshEntity. This means that one cannot use an iterator to
> >>>> modify the mesh. The only place I can think of where this makes sense
> >>>> (except perhaps internally in TopologyComputation) is to change
> >>>> coordinates, and in that case we can require that one uses
> >>>> mesh.geometry().
> >>>>
> >>> It was in MeshSmoothing that I ran into trouble and added a const iterator.
> >> ok, I'll take a look.
> >>
> >>> I would be nice to prioritise what we need to do to get DOLFIN working 
> >>> again so we don't digress too far from this.
> >> I agree, but would like to spend a few more hours on working out the
> >> constness.
> >>
> > 
> > We'll give you a few hours ;). A few days/weeks/months/years would have 
> > been a problem.
> > 
> > Garth

It didn't take that long. We now have const in all the essential
places: Assembler, Function, FunctionSpace, DofMap. From there it
should propagate well throughout the library.

There are still some const_casts to sort out, but we may now continue
to get the assembly back in working order.

-- 
Anders

Attachment: signature.asc
Description: Digital signature