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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 > [email protected] > http://fenics.org/mailman/listinfo/ufc-dev > > -- Ola Skavhaug _______________________________________________ DOLFIN-dev mailing list [email protected] http://www.fenics.org/mailman/listinfo/dolfin-dev
