← Back to team overview

dolfin team mailing list archive

Re: Patch for pyDolfin function.py

 

On Fri, Aug 22, 2008 at 11:12:36AM +0200, Johan Hake wrote:
> On Friday 22 August 2008 11:03:04 Martin Sandve Alnæs wrote:
> > 2008/8/22 Johan Hake <hake@xxxxxxxxx>:
> > > On Friday 22 August 2008 10:25:01 Anders Logg wrote:
> > >> On Fri, Aug 22, 2008 at 10:08:07AM +0200, Martin Sandve Alnæs wrote:
> > >> > 2008/8/22 Anders Logg <logg@xxxxxxxxx>:
> > >> > > On Thu, Aug 21, 2008 at 11:43:41PM +0200, Johan Hake wrote:
> > >> > >> On Thursday 21 August 2008 23:05:57 Evan Lezar wrote:
> > >> > >> > Hi
> > >> > >> >
> > >> > >> > I added a simple warning message to the Function constructor in
> > >> > >> > function.py which is displayed if the constructor is called with
> > >> > >> > 3 arguments and the last one is not an instance of dolfin.Vector
> > >> > >> > or Matrix.
> > >> > >> >
> > >> > >> > I know that a warning is not a necessarily a long-term solution,
> > >> > >> > but it would at least assist new users such as myself in tracking
> > >> > >> > down the source of errors in their code.
> > >> > >>
> > >> > >> Fine that you found out what your problem was!
> > >> > >>
> > >> > >> Your issue again illustrates the limitation Function in pydolfin.
> > >> > >> We should really update at least the docstrings, together with more
> > >> > >> exstensive testings. For example could these lines be expanded:
> > >> > >>
> > >> > >>      # Otherwise give all to DOLFIN
> > >> > >>      else:
> > >> > >>          dolfin.cpp_Function.__init__(self, *((element,) + others))
> > >> > >>
> > >> > >> so that we only send valid arguments to the cpp_Function
> > >> > >> constructor.
> > >> > >>
> > >> > >> A while ago I implemented a way of instantiate a discrete function
> > >> > >> using a numpy array. I promised Martin to send a patch to him for
> > >> > >> inclusion but faild to do it. Please apply Evans patch then I can
> > >> > >> hand a patch for the numpy array initialization after that.
> > >> > >
> > >> > > Done.
> > >> >
> > >> > This check is wrong in many ways:
> > >> >
> > >> >         # Check arguments
> > >> >         if len(others) == 2:
> > >> >             if not isinstance(others[1], (dolfin.Vector,
> > >> > dolfin.GenericVector)): error("Coefficients should be dolfin Vector or
> > >> > Matrix")
> > >> >
> > >> > First, the len(others) == 2 is a very very ugly hack that hides
> > >> > some of the dolfin::Function::Function signatures.
> > >> >
> > >> > Second, checking for dolfin.Vector is superfluous and misleading for
> > >> > people reading the code (they might repeat the practice and forget
> > >> > the GenericVector part in other code).
> > >> >
> > >> > Third, the term "Coefficients" in the message has no clear no meaning.
> > >> >
> > >> > Fourth, "or Matrix" should of course be removed.
> > >> >
> > >> > I'll leave it up to those who use dolfin.Function to fix this mess the
> > >> > way they want it.
> > >>
> > >> Yes, you are right. The Function class in PyDOLFIN is a hack. It needs
> > >> to be fixed, but probably won't before UFL is ready.
> > >>
> > >> But other than the first special case where we need to create a form
> > >> to build a dof map, the two other cases look fine to me:
> > >>
> > >>   # If we have an element, then give element to FFC and the rest to
> > >> DOLFIN elif isinstance(element, (ffc.FiniteElement, ffc.MixedElement)):
> > >> ffc.Function.__init__(self, element)
> > >>       dolfin.cpp_Function.__init__(self, *others)
> > >>   # Otherwise give all to DOLFIN
> > >>    else:
> > >>       dolfin.cpp_Function.__init__(self, *((element,) + others))
> > >>
> > >> SWIG should take care of checking the arguments (*others) against the
> > >> constructors of Function in DOLFIN C++. I don't know what goes wrong,
> > >> but it seems a typemap gets in the way.
> > >
> > > I think that it have to be the "python sequence" to Array<uint/double>
> > > typemap I added by request from Martin that kicks in.
> >
> > Seems right.
> >
> > > If we add a check for numpy array in addition to GenericVector and Vector
> > > in the first test, and then create a Vector from the numpy array, we can
> > > avoid this.
> >
> > No, that's not correct. The constant vector function constructor
> > should take numpy arrays or tuples, not Vector.
> 
> Is it possible to check the length of the numpy array and compare this with 
> the type of element and the size of the mesh to and try to "figure" out if 
> the numpy array should be a Vector() or Array<>?
> 
> > > Shall I send a patch?
> >
> > It's not really a big deal, is it? I say let it be until it's fixed
> > properly.
> 
> Well, sending a numpy array to a Function is quite intuitive for initialize a 
> discrete function, at least for first timers that is used to numpy and 
> Python. And the result Evan got was definetly not a nice error for this 
> guess.

The problem with this is that then one expects that when the Function
is modified, then the array will be modified and vice versa. It won't
since a Vector will be created with copied data.

-- 
Anders


> We could add in the docstring that if you send a Vector you create a discrete 
> function and if you send a numpy sequence then you create a constant 
> function?
> 
> Johan
> _______________________________________________
> DOLFIN-dev mailing list
> DOLFIN-dev@xxxxxxxxxx
> http://www.fenics.org/mailman/listinfo/dolfin-dev

Attachment: signature.asc
Description: Digital signature


Follow ups

References