dolfin team mailing list archive

[Ola Skavhaug <skavhaug@xxxxxxxxx>]

Why not go for Martin's suggestion?

Inheritance is a well known concept in C++, and it avoids using the
evil macros that notoriously break compilation when people forget
their ifndefs. Extending ufc with subclasss in a namespace, say eufc,
dynamic casting in experimental assemblers when needed, and finally,
moving functionality up the inheritance chain when settled seems to me
a better solution than what is happening now?

Ola

On Wed, Apr 29, 2009 at 1:36 PM, Anders Logg <logg@xxxxxxxxx> 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. And at the top
> of that file, we put
>
> #ifdef UFC_EXPERIMENTAL
> #include "ufce.h"
> #else
> #include <ufc.h>
> #endif
>
> That should work.
>
> --
> Anders
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
>
> iEYEARECAAYFAkn4O6IACgkQTuwUCDsYZdG3bQCglt8WQtlFN3U/s73vkEr+rS0E
> lYIAn0tSAnDOgHgyEfnE48ZMFJecjfB8
> =p76f
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> UFC-dev mailing list
> UFC-dev@xxxxxxxxxx
> http://fenics.org/mailman/listinfo/ufc-dev
>
>



-- 
Ola Skavhaug