dolfin team mailing list archive

[Kent Andre <kent-and@xxxxxxxxx>]

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. 
> 
> 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 ? 

Kent