dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

On Wed, Apr 29, 2009 at 01:46:40PM +0200, Kent Andre wrote:
> On on., 2009-04-29 at 12:38 +0100, Garth N. Wells wrote:
> > 
> > Anders Logg wrote:
> > > On Wed, Apr 29, 2009 at 08:34:43AM +0200, Martin Sandve Alnæs wrote:
> > >> On Wed, Apr 29, 2009 at 4:50 AM, Shawn Walker <walker@xxxxxxxxxxxxxxx> wrote:
> > >>>
> > >>> On Tue, 28 Apr 2009, Anders Logg wrote:
> > >>>
> > >>>> On Mon, Apr 27, 2009 at 11:39:08PM -0400, Shawn Walker wrote:
> > >>>>>
> > >>>>> On Mon, 27 Apr 2009, Martin Sandve Aln?s wrote:
> > >>>>>
> > >>>>>> On Mon, Apr 27, 2009 at 3:32 PM, Garth N. Wells <gnw20@xxxxxxxxx> wrote:
> > >>>>>>>
> > >>>>>>> Anders Logg wrote:
> > >>>>>>>> On Mon, Apr 27, 2009 at 09:21:58AM -0400, Shawn Walker wrote:
> > >>>>>>>>> On Mon, 27 Apr 2009, Anders Logg wrote:
> > >>>>>>>>>
> > >>>>>>>>>> On Mon, Apr 27, 2009 at 11:26:13AM +0200, Kent Andre wrote:
> > >>>>>>>>>>> On l?., 2009-04-25 at 00:14 +0200, Anders Logg wrote:
> > >>>>>>>>>>>> On Wed, Apr 22, 2009 at 05:28:30PM -0400, Shawn Walker wrote:
> > >>>>>>>>>>>>> Here is the changeset that adds a `higher_order_coordinates'
> > >>>>>>>>>>>>> variable for
> > >>>>>>>>>>>>> storing higher order mesh data.  This is a very minor change so
> > >>>>>>>>>>>>> please
> > >>>>>>>>>>>>> push this.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> A changeset for DOLFIN is coming immediately after this.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> - Shawn
> > >>>>>>>>>>>> I'm not sure what to do about this. It's problematic to add
> > >>>>>>>>>>>> experimental work to UFC since it must be stable. In particular,
> > >>>>>>>>>>>> any
> > >>>>>>>>>>>> small change to ufc.h means that all forms must be recompiled
> > >>>>>>>>>>>> everywhere for everyone.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> So before we make a change to UFC, we need to know exactly what we
> > >>>>>>>>>>>> need. Which also means I can't import your DOLFIN patch since it
> > >>>>>>>>>>>> depends on the UFC patch.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> I see you've added
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>     double** higher_order_coordinates;
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> to ufc::cell. This is analogous to what is now implemented in
> > >>>>>>>>>>>> MeshGeometry and the mesh XML format so I think it's good.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> The question is what other information we need. As it works now
> > >>>>>>>>>>>> (for
> > >>>>>>>>>>>> the standard ufc::cell), UFC code generated by a form compiler
> > >>>>>>>>>>>> knows
> > >>>>>>>>>>>> what to expect from for a ufc::cell argument. If higher order
> > >>>>>>>>>>>> mappings
> > >>>>>>>>>>>> should work the same way, then the generated code and thus the
> > >>>>>>>>>>>> form
> > >>>>>>>>>>>> compilers need to know which mapping should be used and also the
> > >>>>>>>>>>>> length of higher_order_coordinates. Is this what you were
> > >>>>>>>>>>>> thinking?
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Before we do much more about it, more people need to weigh in on
> > >>>>>>>>>>>> it as
> > >>>>>>>>>>>> it affects DOLFIN, UFC, SyFi and FFC.
> > >>>>>>>>>>>>
> > >>>>>>>>>>> But is there any other way around this. It would be nice with
> > >>>>>>>>>>> higher
> > >>>>>>>>>>> order meshes and UFC should not stop this.
> > >>>>>>>>>>>
> > >>>>>>>>>>> An alternative to changing the cell class would be to make a
> > >>>>>>>>>>> subclass
> > >>>>>>>>>>> of cell. Would this work ?
> > >>>>>>>>>> How about just using the current ufc::cell data structure as it is
> > >>>>>>>>>> but
> > >>>>>>>>>> let coordinates hold all the coordinates?
> > >>>>>>>>>>
> > >>>>>>>>>> This could also be the final solution. Then everything that's needed
> > >>>>>>>>>> is an extra argument to tabulate_tensor that tells the generated
> > >>>>>>>>>> code
> > >>>>>>>>>> whether the cell is affinely mapped or not. The flag could simply be
> > >>>>>>>>>> an integer: 1 means affine, 2 means quadratic etc.
> > >>>>>>>>> But you still need to modify the ufc::cell code, I think.  There is
> > >>>>>>>>> also
> > >>>>>>>>> an implicit assumption that the higher order coordinates should
> > >>>>>>>>> contain
> > >>>>>>>>> the standard mesh vertex coordinates.  Of course, this is true for
> > >>>>>>>>> most
> > >>>>>>>>> practical cases.  But for more fancy mappings, maybe this is not the
> > >>>>>>>>> case.
> > >>>>>>>> It seems to me that a reasonable assumption would be to limit the
> > >>>>>>>> cases to P1, P2, P3, etc, that is, mappings that can be written down
> > >>>>>>>> using standard Lagrange bases so then the vertices will always be
> > >>>>>>>> included. They would also be first in the list meaning that the code
> > >>>>>>>> would actually work (but might not give accurate results) even if it
> > >>>>>>>> were generated for affine mappings.
> > >>>>>>>>
> > >>>>>>>>> Also, in the ufc::cell code, you currently read in the cell
> > >>>>>>>>> coordinates
> > >>>>>>>>> using info in MeshTopology.  However, the higher order coordinate
> > >>>>>>>>> info
> > >>>>>>>>> resides in MeshGeometry (which is where it belongs).  So you would
> > >>>>>>>>> still
> > >>>>>>>>> need to modify ufc.h.   Remember, there is higher order cell data
> > >>>>>>>>> that is
> > >>>>>>>>> contained in MeshGeometry.
> > >>>>>>>> Where is MeshTopology used for this? I looked in UFCCell.h which is
> > >>>>>>>> where the coordinates are copied to ufc::cell and there MeshGeometry
> > >>>>>>>> is used.
> > >>>>>>>>
> > >>>>>>>>> Is it really that hard to change ufc.h?  Other things have to be
> > >>>>>>>>> recompiled, but isn't that automatic?
> > >>>>>>>> Yes, it's easy to change, but a main point with UFC is that we
> > >>>>>>>> shouldn't change it.
> > >>>>>>>>
> > >>>>>>> UFC will need to be extended as time goes on, but it is hard to know
> > >>>>>>> from the outset how it should be done. What about using some IFDEF's or
> > >>>>>>> non-pure virtual functions in the development version to allow
> > >>>>>>> experimentation? These can then either be removed or added to UFC at
> > >>>>>>> release time.
> > >>>>>>>
> > >>>>>>> Garth
> > >>>>>> Or subclasses with non-pure virtual functions:
> > >>>>>>
> > >>>>>> class experimental_cell_integral: public ufc::cell_integral
> > >>>>>> {
> > >>>>>>  void foo() const { throw ...("Experimental feature not implemented.");
> > >>>>>> }
> > >>>>>> };
> > >>>>>>
> > >>>>>> or
> > >>>>>>
> > >>>>>> namespace eufc {
> > >>>>>>  class cell_integral: public ufc::cell_integral
> > >>>>>>  {
> > >>>>>>   void foo() const { throw ...("Experimental feature not implemented.");
> > >>>>>> }
> > >>>>>>  };
> > >>>>>> }
> > >>>>>>
> > >>>>>> We can define these in "experimental_ufc.h" or "eufc.h" to keep the
> > >>>>>> official header file constant.
> > >>>>>>
> > >>>>>> Then the DOLFIN code that uses experimental features must be clearly
> > >>>>>> marked:
> > >>>>>>
> > >>>>>>  ufc::cell_integral *itg = form.create_cell_integral(0)
> > >>>>>>  eufc::cell_integral *eitg = dynamic_cast<eufc::cell_integral>(itg);
> > >>>>>>
> > >>>>>> and can then use "if(eitg)" to select between experimental and
> > >>>>>> non-experimental code.
> > >>>>> In a similar vein, could we just have another file named eufc.h, and put
> > >>>>> an IFNDEF somewhere that would use eufc.h instead of ufc.h?  That way I
> > >>>>> could modify eufc.h all I want, and people don't have to use it.  But I'm
> > >>>>> not sure how to do this.  Ideally, this would be an option for scons like
> > >>>>> enableExpUFC=true.  Or is it only necessary to include eufc.h in files
> > >>>>> that FFC generates?  We just need something for testing.
> > >>>>>
> > >>>>> Obviously, I cannot just modify UFCCell.h.  I tried that, but FFC cannot
> > >>>>> access variables declared in the sub-class (woops...  :(  ).
> > >>>> One option could be to create a file named ufc.h and put it in
> > >>>>
> > >>>>  dolfin/fem/ufc.h
> > >>>>
> > >>>> and change all #include <ufc.h> to #include "ufc.h".
> > >>>>
> > >>>> Then the DOLFIN version of ufc.h will include either the installed
> > >>>> ufc.h or another file named ufce.h which is placed in
> > >>>>
> > >>>>  dolfin/fem/ufce.h
> > >>>>
> > >>>> That file contains data structures named the same way as in the
> > >>>> official ufc.h but with modifications (so the rest of the code won't
> > >>>> need to be changed much).
> > >>>>
> > >>>> Then in the DOLFIN ufc.h control file we place an #ifdef for whether
> > >>>> to include the official ufc.h or the experimental one:
> > >>>>
> > >>>> #ifdef UFC_EXPERIMENTAL
> > >>>> #include "ufce.h"
> > >>>> #else
> > >>>> #include <ufc.h>
> > >>>> #endif
> > >>>>
> > >>>> You can set the flag by adding
> > >>>>
> > >>>>  customCxxFlags="-DUFC_EXPERIMENTAL"
> > >>>>
> > >>>> to scons.
> > >>> Wouldn't it make more sense to have this exp_ufc.h be a part of ufc?  That
> > >>> way when you install ufc, there can be an `enableExpUFC' option.  In this
> > >>> case, scons will copy exp_ufc.h to the build directory and rename it to
> > >>> ufc.h.  The exp_ufc.h file will basically be identical to ufc.h, so it makes
> > >>> sense to keep it together.  This will also put in a convenient `buffer' for
> > >>> experimenting with ufc, and allow for easy moving over of additions to
> > >>> ufc.h.  Could someone please put this in?  Please?  :)
> > > 
> > > It's better to have it in DOLFIN. It makes it easier to experiment (no
> > > need to change UFC). When features have stabilized, we move it to UFC.
> > > 
> > > I agree having both ufc.h and UFC.h is confusing. How about this
> > > instead. In DOLFIN, we never include ufc.h, only UFC.h. 

What I meant was, we need to change this so we only include UFC.h in
places where we now include ufc.h. The inclusion of ufc.h will be
handled by UFC.h and it will pick the right one.

> > We do include ufc.h in a number of places. This needs to be fixed so 
> > that a ufc(e).h file can be placed in DOLFIN.
> > 
> > Garth
> > 
> 
> What about generated code ? 

The experimental form compiler or the person hand-writing the
experimental code would need to handle that.

-- 
Anders

Attachment: signature.asc
Description: Digital signature