On Tue, Apr 26, 2011 at 09:24:02AM -0700, Johan Hake wrote: > 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.h...@gmail.com> 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.
Great! Will fix soon. > Still think form.compute_form_data() line is superflous ;) I foresee we will have another long discussion in a year or two and we will then end up removing compute_form_data... :-) -- Anders _______________________________________________ Mailing list: https://launchpad.net/~ffc Post to : ffc@lists.launchpad.net Unsubscribe : https://launchpad.net/~ffc More help : https://help.launchpad.net/ListHelp
