dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

On Mon, Feb 16, 2009 at 09:49:49PM +0000, A Navaei wrote:
> 2009/2/16 Anders Logg <logg@xxxxxxxxx>:
> > On Mon, Feb 16, 2009 at 09:37:38PM +0000, A Navaei wrote:
> >> 2009/2/16 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.
> >>
> >> If it doesn't mess up other dependencies, that would be indeed helpful.
> >
> > ok. I'll wait for Garth to comment before I make the changes.
> >
> >> Also, I'm curious to know why some variables are returned by value and
> >> some by reference, eg, function_space() and function_space_ptr(), and
> >> why the same thing happens for some ctors, eg, Function(const
> >> FunctionSpace V&) and Function(boost::shared_ptr< FunctionSpace > V).
> >
> > Which are returned by value? Everything that is not a built-in type
> > (int, float etc) is returned by reference to avoid copying.
> 
> I mean this:
> 
>     /// Return the function space
>     const FunctionSpace& function_space() const;
> 
>     /// Return the function space
>     boost::shared_ptr<const FunctionSpace> function_space_ptr() const;
> 
> Is this for compatibility with codes written prior to the appearance
> of shared pointers?

It's to provide a choice between the two. References are supported
natively by C++ and allows cleaner, nicer, simpler syntax than shared
pointers so this is the default. But when shared pointers are needed
one can choose to use those.

-- 
Anders

Attachment: signature.asc
Description: Digital signature