dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

On Mon, Feb 16, 2009 at 09:15:21PM +0000, A Navaei wrote:
> 2009/2/16 Anders Logg <logg@xxxxxxxxx>:
> > On Mon, Feb 16, 2009 at 09:59:01PM +0100, Martin Sandve Alnæs wrote:
> >> On Mon, Feb 16, 2009 at 9:39 PM, Anders Logg <logg@xxxxxxxxx> wrote:
> >> > On Mon, Feb 16, 2009 at 06:11:41PM +0000, A Navaei wrote:
> >> >> 2009/2/16 A Navaei <axnavaei@xxxxxxxxxxxxxx>:
> >> >> > 2009/2/16 Anders Logg <logg@xxxxxxxxx>:
> >> >> >> On Mon, Feb 16, 2009 at 05:36:48PM +0000, Garth N. Wells wrote:
> >> >> >>>
> >> >> >>>
> >> >> >>> A Navaei wrote:
> >> >> >>> > 2009/2/16 Anders Logg <logg@xxxxxxxxx>:
> >> >> >>> >> On Mon, Feb 16, 2009 at 04:36:21PM +0000, A Navaei wrote:
> >> >> >>> >>> 2009/2/15 Anders Logg <logg@xxxxxxxxx>:
> >> >> >>> >>>> On Sat, Feb 14, 2009 at 06:44:05PM +0000, Garth N. Wells wrote:
> >> >> >>> >>>>>
> >> >> >>> >>>>> A Navaei wrote:
> >> >> >>> >>>>>> [snip]
> >> >> >>> >>>>>>>>> Function should not change the FunctionSpace (that's why FunctionSpace is
> >> >> >>> >>>>>>>>> const). FunctionSpace shouldn't depend on the data and its size should be
> >> >> >>> >>>>>>>>> defined when creating the FunctionSpace.
> >> >> >>> >>>>>>>> The same applies for FunctionSpace as its member variables are private
> >> >> >>> >>>>>>>> and the public accessors are read-only which. Consider a sub-class
> >> >> >>> >>>>>>>> ImageFunctionSpace:FunctionSpace, with a constructor like:
> >> >> >>> >>>>>>>>
> >> >> >>> >>>>>>>> ImageFunctionSpace(ImageType *imagePtr)
> >> >> >>> >>>>>>>>
> >> >> >>> >>>>>>>> where imagePtr is supposed to initialise FunctionSpace::_mesh using
> >> >> >>> >>>>>>>> the image size, and then _dofmaps itself is initialised using _mesh.
> >> >> >>> >>>>>>>> How would you do that considering the restrictions?
> >> >> >>> >>>>>>>>
> >> >> >>> >>>>>>> The FunctionSpace has pointers to the mesh, etc. You just need to create
> >> >> >>> >>>>>>> your mesh and pass it to the FunctionSpace constructor. What else you then
> >> >> >>> >>>>>>> do with the mesh, etc is up to you.
> >> >> >>> >>>>>> That can be done _outside_ of a sub-class. A sub-class of
> >> >> >>> >>>>>> FunctionSpace doesn't have a control over _mesh of its own parent
> >> >> >>> >>>>>> FunctionSpace. The following example may make this more clear:
> >> >> >>> >>>>>>
> >> >> >>> >>>>>> template <typename TImage>
> >> >> >>> >>>>>> class DolfinImageFunctionSpace : public dolfin::FunctionSpace
> >> >> >>> >>>>>> {
> >> >> >>> >>>>>> public:
> >> >> >>> >>>>>>         // just some itk typedefs -- ignore
> >> >> >>> >>>>>>     typedef TImage ImageType;
> >> >> >>> >>>>>>     typedef typename ImageType::PixelType PixelType;
> >> >> >>> >>>>>>     typedef typename ImageType::SizeType SizeType;
> >> >> >>> >>>>>>
> >> >> >>> >>>>>>         // .. and some dolfin typedefs -- ignore
> >> >> >>> >>>>>>     typedef typename std::tr1::shared_ptr<const dolfin::Mesh> MeshConstPointerType;
> >> >> >>> >>>>>>     typedef typename std::tr1::shared_ptr<const dolfin::FiniteElement>
> >> >> >>> >>>>>> ElementConstPointerType;
> >> >> >>> >>>>>>     typedef typename std::tr1::shared_ptr<const dolfin::DofMap>
> >> >> >>> >>>>>> DofMapConstPointerType;
> >> >> >>> >>>>>>
> >> >> >>> >>>>>>         // the ctor
> >> >> >>> >>>>>>     DolfinImageFunctionSpace(ImageType* imageData,
> >> >> >>> >>>>>>                                             MeshConstPointerType mesh,
> >> >> >>> >>>>>>                                             ElementConstPointerType element,
> >> >> >>> >>>>>>                                             DofMapConstPointerType dofmap) :
> >> >> >>> >>>>>>     dolfin::FunctionSpace(mesh, element, dofmap)
> >> >> >>> >>>>>>     {
> >> >> >>> >>>>>>             SizeType imageSize = imageData->GetBufferedRegion().GetSize();
> >> >> >>> >>>>>>
> >> >> >>> >>>>>>                 // here, we whish to call some thing like:
> >> >> >>> >>>>>>                 // _mesh = UnitSquare(imageSize[0], imageSize[1]);
> >> >> >>> >>>>>>                 // but it's private and the accessor is read-only.
> >> >> >>> >>>>>>     };
> >> >> >>> >>>>>> }
> >> >> >>> >>>>>>
> >> >> >>> >>>>> This breaks the concept of a function space. A function space is defined
> >> >> >>> >>>>> in terms of a mesh (and other things). A function space does not define
> >> >> >>> >>>>> its mesh.
> >> >> >>> >>>>>
> >> >> >>> >>>>> It looks to me like the class that you're creating should be called
> >> >> >>> >>>>> something like
> >> >> >>> >>>>>
> >> >> >>> >>>>>    DolfinImageProblem,
> >> >> >>> >>>>>
> >> >> >>> >>>>> which can create its own Mesh, FunctionSpace and other objects.
> >> >> >>> >>>>>
> >> >> >>> >>>>> Garth
> >> >> >>> >>>>>
> >> >> >>> >>>>>> -Ali
> >> >> >>> >>>> I think what you need to do is something like this:
> >> >> >>> >>>>
> >> >> >>> >>>> 1. Create a subclass of Function
> >> >> >>> >>>>
> >> >> >>> >>>> 2. In the constructor of your Function, call the empty Function()
> >> >> >>> >>>> constructor
> >> >> >>> >>>>
> >> >> >>> >>>> 3. Then, still in the constructor of your Function (not
> >> >> >>> >>>> FunctionSpace), create everything necessary like figuring out the
> >> >> >>> >>>> mesh, dofmap etc and from that create a FunctionSpace
> >> >> >>> >>>>
> >> >> >>> >>>> 4. Then use that FunctionSpace (still in the constructor of your
> >> >> >>> >>>> Function subclass) to create a new Function v which uses your special
> >> >> >>> >>>> FunctionSpace in its constructor
> >> >> >>> >>>>
> >> >> >>> >>>> 5. Finally, assign this function to the created Function:
> >> >> >>> >>>>
> >> >> >>> >>>>  *this = v;
> >> >> >>> >>>  error: no match for 'operator=' in
> >> >> >>> >>> '*(itk::DolfinImageFunction<itk::Image<double, 2u> >*)this = v'
> >> >> >>> >>>
> >> >> >>> >>> Assigning Function to ImageFunction is a trouble, see the full code here:
> >> >> >>> >>>
> >> >> >>> >>> http://code.google.com/p/wrapitk/source/browse/trunk/ExternalProjects/ItkDolfin/src/itkDolfinImageFunction.h#87
> >> >> >>> >> Do you have the latest hg version?
> >> >> >>> >>
> >> >> >>> >> Function assignment should work (see recent discussion on the mailing
> >> >> >>> >> list on copy constructors and assignment operators for Function).
> >> >> >>> >>
> >> >> >>> >>> I don't understand the philosophy behind this tight security: why no
> >> >> >>> >>> protected member variables? Why is everything either public or
> >> >> >>> >>> private? Needless to say, the protected members were designed to allow
> >> >> >>> >>> class extensions, by banning it, you're making sub classing a
> >> >> >>> >>> unnecessarily complicated task. The above problem has it's own work
> >> >> >>> >>> arounds, but I don't understand why the obvious way is blocked.
> >> >> >>> >> The reason is simply that the constructor arguments in Function and
> >> >> >>> >> FunctionSpace are const. This means no one is allowed to change the
> >> >> >>> >> variables, not even the Function class (or FunctionSpace class)
> >> >> >>> >> itself. For example, it's reasonable when you create a Function on a
> >> >> >>> >> FunctionSpace that the Function will not change the FunctionSpace.
> >> >> >>> >
> >> >> >>> > Whether the member variables are read-only or not, and their
> >> >> >>> > visibility are two separate concepts - you can have any combination of
> >> >> >>> > these. Function and FunctionSpace do change their const variables
> >> >> >>> > anyway, eg, in operator= you have:
> >> >> >>> >
> >> >> >>> > // Assign vector
> >> >> >>> > init();
> >> >> >>> > dolfin_assert(_vector);
> >> >> >>> > *_vector = *v._vector;
> >> >> >>> >
> >> >> >>> > Which is in contradiction with what you wrote about not even the class
> >> >> >>> > itself cannot change the variables.
> >> >> >>> >
> >> >> >>>
> >> >> >>> Anders was referring to FunctionSpace. Obviously, the vector associated
> >> >> >>> with a discrete Function cannot be const (and it isn't).
> >> >> >>>
> >> >> >>> > It would be rare that all classes have to have read-only inputs, where
> >> >> >>> > the sub-classes are setences to inherit the same properties. Now, I
> >> >> >>> > simply cannot have the right operator= assinging Function to
> >> >> >>> > ImageFunction implemented, since I cannot assign anything to the
> >> >> >>> > private member variables.
> >> >> >>> >
> >> >> >>> > If the visibility of the members variables is not going to change to
> >> >> >>> > protected, or at least a protected read/write accessor is not
> >> >> >>> > provided, then there will be no option but implementing everything in
> >> >> >>> > a third-party class faking the current ImageFunction, as it's not
> >> >> >>> > going to be derived from Function.
> >> >> >>> >
> >> >> >>>
> >> >> >>> All you need to do is create a suitable FunctionSpace before creating
> >> >> >>> your Function.
> >> >> >>>
> >> >> >>> Garth
> >> >> >>
> >> >> >> Yes, and you should be able to create that FunctionSpace inside the
> >> >> >> constructor for your Function subclass.
> >> >> >
> >> >> > Yes, I can _create_ it, but I cannot _assign_ it :)
> >> >> >
> >> >> > Now I'm trying this work around:
> >> >> >
> >> >> > DolfinImageFunction(ImageType* imageData) :
> >> >> >                        dolfin::Function()
> >> >> > {
> >> >> > ...
> >> >> >        // Create a Function instance
> >> >> >        FSConstPointerType V = CreateFunctionSpace();
> >> >> >        DolfinImageFunction v(V);
> >> >> >        *this = v;
> >> >> > };
> >> >> >
> >> >> > which requires the repsence of this ctor:
> >> >> >
> >> >> > DolfinImageFunction(boost::shared_ptr<const dolfin::FunctionSpace> V):
> >> >> >        dolfin::Function(V)
> >> >> > {
> >> >> > };
> >> >> >
> >> >> > This might work.
> >> >
> >> > Yes, that's what I suggested (but maybe I didn't explain it too well).
> >> >
> >> >> The above builds, but then I get thos runtime error:
> >> >>
> >> >> RuntimeError: *** Error: Cannot copy Functions which do not have a
> >> >> Vector (user-defined Functions).
> >> >>
> >> >> The error originates from 'this' instance and not from v.
> >> >>
> >> >> Just in case, here is the full code:
> >> >>
> >> >> http://code.google.com/p/wrapitk/source/browse/trunk/ExternalProjects/ItkDolfin/src/itkDolfinImageFunction.h#47
> >> >
> >> > Yes, this is expected and in accordance with another thread today on
> >> > assignment of Functions. It is not possible to assign Functions which
> >> > are only defined by an eval() operator (like in your case).
> >> >
> >> > The real problem here is that C++ does not allow anything to happen
> >> > between the call to the contstructor and the initialization of the
> >> > base class. What you really want to do is this:
> >> >
> >> >  DolfinImageFunction(ImageType* imageData)
> >> >  {
> >> >    // Create function space
> >> >    FSConstPointerType V = CreateFunctionSpace(imageData);
> >> >
> >> >    // Initialize base class
> >> >    Function(V);
> >> >  }
> >> >
> >> > This is possible in Python but not C++ as the call to the base class
> >> > constructor must happen before the constructor body.
> >> >
> >> > The only solution I see is to add an init() function to Function that
> >> > allows a user to set the FunctionSpace. Either we name it init() as
> >> > we've done in many other DOLFIN classes, or we add a function named
> >> > set_function_space() which would match has_function_space().
> >>
> >> You still haven't explained why making member variables protected
> >> instead of private is not an option, as was asked for?
> >> Might be a good reason, but I'm curious too what that reason is.
> >
> > I'd like all member variables to be private unless there's a very good
> > reason not to make them private.
> 
> Here is one good reason: to allow others to extend the class :)
> 
> > The Function/FunctionSpace classes
> > are particularly intricate with a _vector that may or may not be null
> > depending on the current state of the Function, so by allowing access
> > only through public functions we can perform checks (which we could do
> > better) and some magic. For example, calling vector() does not only
> > return the vector instance, it also creates the vector and initializes
> > it with the correct size if necessary.
> 
> Why not following the standard design pattern of accessors used by
> many other people?

The Function class has been designed for extension in one particular
way, namely by overloading the eval() function. However, I'm starting
to believe it might make sense to make the two member variables in
Function protected. Would that be enough? I still don't think it's a
good idea to do the same for FunctionSpace as there are quite a few
more member variables.

-- 
Anders

Attachment: signature.asc
Description: Digital signature