dolfin team mailing list archive

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

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.

Martin