ffc team mailing list archive

[Johan Hake <johan.hake@xxxxxxxxx>]

On Tuesday April 26 2011 09:18:15 Garth N. Wells wrote:
> On 26/04/11 17:16, Anders Logg wrote:
> > On Tue, Apr 26, 2011 at 06:12:26PM +0200, Martin Sandve Alnæs wrote:
> >> On 26 April 2011 18:10, Johan Hake <johan.hake@xxxxxxxxx> wrote:
> >>     On Tuesday April 26 2011 09:01:35 Anders Logg wrote:
> >>     > On Tue, Apr 26, 2011 at 08:44:24AM -0700, Johan Hake wrote:
> >>     > > On Tuesday April 26 2011 08:42:32 Anders Logg wrote:
> >>     > > > On Tue, Apr 26, 2011 at 08:39:30AM -0700, Johan Hake wrote:
> >>     > > > > On Tuesday April 26 2011 08:33:11 Garth N. Wells wrote:
> >>     > > > > > On 26/04/11 16:31, Johan Hake wrote:
> >>     > > > > > > On Tuesday April 26 2011 08:16:29 Garth N. Wells wrote:
> >>     > > > > > >> On 26/04/11 16:07, Anders Logg wrote:
> >>     > > > > > >>> On Tue, Apr 26, 2011 at 03:59:52PM +0100, Garth N.
> >>     > > > > > >>> Wells
> >>     
> >>     wrote:
> >>     > > > > > >>>> On 26/04/11 15:55, Anders Logg wrote:
> >>     > > > > > >>>>> On Tue, Apr 26, 2011 at 03:45:22PM +0100, Garth N.
> >>     > > > > > >>>>> Wells
> >>     
> >>     wrote:
> >>     > > > > > >>>>>> On 26/04/11 13:51, Anders Logg wrote:
> >>     > > > > > >>>>>>> On Tue, Apr 26, 2011 at 02:00:50PM +0200, Anders
> >>     > > > > > >>>>>>> Logg
> >>     
> >>     wrote:
> >>     > > > > > >>>>>>>> It feels good that you trust me enough to handle
> >>     > > > > > >>>>>>>> it. ;-)
> >>     > > > > > >>>>>>>> 
> >>     > > > > > >>>>>>>> Will add it sometime this afternoon and then we
> >>     > > > > > >>>>>>>> can revisit the JIT compiler caching.
> >>     > > > > > >>>>>>> 
> >>     > > > > > >>>>>>> I'm getting confused here... Looking at
> >>     > > > > > >>>>>>> preprocess.py in UFL, I see
> >>     > > > > > > 
> >>     > > > > > > this:
> >>     > > > > > >>>>>> It is confusing. Does the function 'preprocess' do
> >>     
> >>     anything
> >>     
> >>     > > > > > >>>>>> that the old FormData class didn't? It would be
> >>     > > > > > >>>>>> easier to follow if Form just had a member
> >>     > > > > > >>>>>> function form_data() that computes and stores data
> >>     > > > > > >>>>>> (like it used to), or if Form had a 'preprocess'
> >>     > > > > > >>>>>> function. Having the function preprocess return a
> >>     > > > > > >>>>>> new form is really confusing.
> >>     > > > > > >>>>> 
> >>     > > > > > >>>>> I don't find that particularly confusing. It's the
> >>     > > > > > >>>>> same as
> >>     > > > > > >>>>> 
> >>     > > > > > >>>>>   refined_mesh = refine(mesh)
> >>     > > > > > >>>> 
> >>     > > > > > >>>> Which is the whole problem. By creating a new object,
> >>     
> >>     FormData
> >>     
> >>     > > > > > >>>> is thrown away. The preprocessing should just compute
> >>     > > > > > >>>> some more data, just like we *don't* do
> >>     > > > > > >>>> 
> >>     > > > > > >>>>   initialised_mesh = mesh.init(0)
> >>     > > > > > >>>> 
> >>     > > > > > >>>> What was wrong with Martin's original design that
> >>     
> >>     necessitated
> >>     
> >>     > > > > > >>>> the change?
> >>     > > > > > >>> 
> >>     > > > > > >>> As I explained, I thought it was better to have an
> >>     > > > > > >>> explicit call to preprocess since that makes it clear
> >>     > > > > > >>> that one makes a call to a function which may take
> >>     > > > > > >>> some time to execute (instead of just calling a
> >>     > > > > > >>> member function which seems to
> >>     
> >>     just
> >>     
> >>     > > > > > >>> return some data).
> >>     > > > > > >>> 
> >>     > > > > > >>> But as I say above: I added the caching back at some
> >>     > > > > > >>> point (maybe even the day after I removed it 2 years
> >>     > > > > > >>> ago) so we don't need to discuss why I removed it (as
> >>     > > > > > >>> I realized myself
> >>     
> >>     I
> >>     
> >>     > > > > > >>> shouldn't have removed it and added it back a long
> >>     > > > > > >>> time ago).
> >>     > > > > > >>> 
> >>     > > > > > >>> What has me confused now is that the caching seems to
> >>     > > > > > >>> be in place but we still need the extra caching in
> >>     > > > > > >>> FFC/DOLFIN and I don't see why.
> >>     > > > > > >> 
> >>     > > > > > >> Because preprocess returns a new form, e.g. define a
> >>     > > > > > >> form
> >>     > > > > > >> 
> >>     > > > > > >>   a = u*v*dx
> >>     > > > > > >>   jit(a)
> >>     > > > > > >> 
> >>     > > > > > >> Inside jit,
> >>     > > > > > >> 
> >>     > > > > > >>    a.form_data() is None:
> >>     > > > > > >>        b = preprocess(a) # b now has data attached, but
> >>     > > > > > >>        a doesn't
> >>     > > > > > >>    
> >>     > > > > > >>    else:
> >>     > > > > > >>        b = a
> >>     > > > > > >> 
> >>     > > > > > >> Now 'b' has been preprocessed, and has form data
> >>     > > > > > >> attached, but 'a' doesn't. Calling 'jit(a)' again, the
> >>     > > > > > >> code will never enter the 'else' part of the clause
> >>     > > > > > >> because 'a' never gets any form data. Johan has added
> >>     > > > > > >> some code FFC that attaches the form
> >>     
> >>     data
> >>     
> >>     > > > > > >> of 'b' to 'a', but it is a bit clumsy.
> >>     > > > > > > 
> >>     > > > > > > No, it was already attached. I just made ffc use it.
> >>     > > > > > 
> >>     > > > > > Didn't you add the line
> >>     > > > > > 
> >>     > > > > >     form._form_data = preprocessed_form.form_data()
> >>     > > > > 
> >>     > > > > No, I added:
> >>     > > > >   preprocessed_form = form.form_data()._form
> >>     > > > > 
> >>     > > > > I think the thing here is that form_data has always had a
> >>     > > > > preprocessed form. Someone (lets not point fingers!) thought
> >>     > > > > that was too much magic and added an
> >>     > > > > 
> >>     > > > > explicit need to call:
> >>     > > > >   form = preprocess(form)
> >>     > > > > 
> >>     > > > > in jit_compiler(). This made the design more complicated and
> >>     > > > > also introduced a cirucular dependency, as the return
> >>     > > > > preprocessed form need to know of its form_data, but the
> >>     > > > > form_data already had a reference to the preprocessed form.
> >>     > > > > The latter is what I used in
> >>     
> >>     the
> >>     
> >>     > > > > one line I altered.
> >>     > > > 
> >>     > > > No, it made the design cleaner since it makes clear something
> >>     > > > needs
> >>     
> >>     to
> >>     
> >>     > > > happen to get the metadata: a call to preprocess.
> >>     > > > 
> >>     > > > Where did you add this line?
> >>     > > 
> >>     > > I change
> >>     > > 
> >>     > >   preprocessed_form = form
> >>     > > 
> >>     > > to:
> >>     > >   preprocessed_form = form.form_data()._form
> >>     > 
> >>     > Yes, but where?
> >>     > 
> >>     > I've fixed the bug now in preprocess.py (attaching to both forms).
> >>     > Does that help?
> >>     
> >>     In ffc.jit_form.
> >>     
> >>     Your fix wont fix the circular dependency.
> >>     
> >>     We also need to remove form_data from the preprocessed form.
> >> 
> >> Or just use a weakref.
> >> 
> >>     This means that
> >>     we need to return form_data from preprocess and maybe change its
> >>     name to compute_form_data.
> >>     
> >>     Johan
> > 
> > Why is the circular dependency a problem? Anyway, I'm thinking now the
> > cleanest design would be
> > 
> >   form.compute_form_data()
> >   form_data = form.form_data()
> >   preprocessed_form = form_data.preprocessed_form
> > 
> > Here, the preprocessed_form would not store form_data.
> 
> Agree.

Go for it! We would then also avoid problems with ciruclar dep.

Still think form.compute_form_data() line is superflous ;)

Johan

> Garth
> 
> > --
> > Anders