dolfin team mailing list archive

["Martin Sandve Alnæs" <martinal@xxxxxxxxx>]

2008/10/26 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

Although you're not doing anything wrong now that I see,
I think it's worth mentioning that any use of const_cast or
mutable may lead to well hidden bugs in a multithreaded
application. In fact, even the C++ rules for const aren't strict
enough, so at least g++ has an additional keyword "pure"
which is even stricter (no sideeffects at all allowed).

--
Martin