Anders Logg wrote: > On Mon, Feb 16, 2009 at 09:37:38PM +0000, A Navaei wrote: >> 2009/2/16 Anders Logg <[email protected]>: >>> On Mon, Feb 16, 2009 at 09:15:21PM +0000, A Navaei wrote: >>>> 2009/2/16 Anders Logg <[email protected]>: >>>>> 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 <[email protected]> wrote: >>>>>>> On Mon, Feb 16, 2009 at 06:11:41PM +0000, A Navaei wrote: >>>>>>>> 2009/2/16 A Navaei <[email protected]>: >>>>>>>>> 2009/2/16 Anders Logg <[email protected]>: >>>>>>>>>> On Mon, Feb 16, 2009 at 05:36:48PM +0000, Garth N. Wells wrote: >>>>>>>>>>> >>>>>>>>>>> A Navaei wrote: >>>>>>>>>>>> 2009/2/16 Anders Logg <[email protected]>: >>>>>>>>>>>>> On Mon, Feb 16, 2009 at 04:36:21PM +0000, A Navaei wrote: >>>>>>>>>>>>>> 2009/2/15 Anders Logg <[email protected]>: >>>>>>>>>>>>>>> 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. >
The pointer to the FunctionSpace in Function could be made protected, but the FunctionSpace should remain const. The vector can already be accessed, so it's fine as it is. Garth >> 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. > > > > ------------------------------------------------------------------------ > > _______________________________________________ > DOLFIN-dev mailing list > [email protected] > http://www.fenics.org/mailman/listinfo/dolfin-dev _______________________________________________ DOLFIN-dev mailing list [email protected] http://www.fenics.org/mailman/listinfo/dolfin-dev
