Anders Logg wrote: > 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. 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. >
It also makes it easier for users to develop against evolving versions of DOLFIN. For example, we recently changed the storage in a number of classes from plain pointers to shared pointers which cannot have affected user-created sub-classes since the data is private. Garth > > > ------------------------------------------------------------------------ > > _______________________________________________ > 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
