#771 configure generated gridtype.hh / dgfgridtype.hh

Christian: I didn't like this patch from it's beginning and think it still needs discussion. I will not merge this patch to the release. It is a significant change.

Oliver: I have an uneasy feeling about this one too. For my taste, it adds a bit too much magic. Even worse, it adds decentral magic. But I admit that this is a subjective judgement.

@Christian & Olvier:

Maybe you need some extra explanation. I cannot understand your arguments (if we like to call uneasy feelings arguments, no offense here). The gridtype header (an the dgfgridtype when using DGF parser) was designed to allow easy change of grid types in a program code. Without that header code would look like foolows (see also #760 (closed) an many others):

#define DIMENSION 2

#if HAVE_ALUGRID #include <dune/grid/alugrid.hh> #endif #if HAVE_UG #include <dune/grid/uggrid.hh> #endif

/// somewhere below typedef ALUSimplexGrid<DIMENSION,DIMENSION> GridType; //typedef UGGrid< DIMENSION > GridType; ....

So the change of GridType used has to be done manually in the source code. This becomes very uggly when one wants to use more than 2 different grids, which we do by the way.

With the use of gridtype.hh the code looks as follows:

#include <gridtype.hh> // this defines GridSelector :: GridType according to defined type and dimension

which is ALUSimplexGrid when using make GRIDTYPE=ALUGRID_SIMPLEX and UGGrid when calling make GRIDTYPE=UGGRID

The source code is not changed when changing the GridType. This is very useful when one wants to use for example all available grids in dune-grid. I have to assume that you don't do that, otherwise, you would have stumbled upon the same problem. We in Freiburg, and I guess also Münster, want to use more than 2 grids and we don't want to change the source code for changing the grid type. Another point is, that with this feature program compilation can be automated by using shell scripts. We use this feature when performing EOC analysis of algorithms under development. It helped us a lot to speed up these things. Another thing is, that with packages like ALBERTA you can automaically choose the right library depending on the griddim selected, and so on.

This was all convered by the existing gridtype.hh and dgfgridtype.hh. Now the following problem arises when an external grid is used, e.g. PrismGrid and other grids developed at our institute. To also use this GridType technique the types for PrismGrid have to be added before the original gridtype.hh is included (because at the end of this file there is a check that would fail otherwise). This is very unsatisfying and caused a lot of problems because for every new grid one has to extra include a gridtype file. So the patch we are talking about solves this problem, because for every grid sugested in the dune.module file (also of other projects that can use the macro to generate these files) the grid type definitions are included into the one gridtype.hh header. Furthermore, it increases maintainability of the code because there is only one code for all grids and one can debug by using one grid and does not have to check all grids. Also, if a grid is not available, there is no grid type definition, which was not possible before and leaded to some strange errors. But the best thing of all, if YOU don't like it, you DON'T HAVE TO USE IT. It will never interfere with other stuff. And why, because WE don't like to patronize other users.

To sum up, this patchs gives us:

more flexibility of an existing and widely used feature
better (easier) maintainability because of
- centralization of code to one file (two because we excluded the DGF stuff)
- removal of redundant code

So please tell me why you don't like one of the four points mentioned above, or why you have uneasy feelings about improvemnts (maybe it's a typlical german thing ;) ).

Best

R

First, I understand the goal, I just don't like the solution.

I still consider the current approach a hack, as it introduces preprocessor magic. After the last changes, even more magic was introduced. The main reason why I rejected the request for merge was, that it isn't working. There are many places in the trunk, where the is not working anymore, like the grid-howto (at least that was the state 3 weeks ago). If I tried to fix the code an followed the instructions in the doxygen-documentation, it still failed, it seems the docs are (or were) a bit outdated.

You were right. The change is a compatibility issue... and that is the reason that is not part of the release.

Now to the current solution...

Up to now the hack was limited to small part of the repository. It was always agreed that it isn't part of the interface. But now after the latest changes all implementations have to provide their own part of the magic.
There are certain things which are not possible with the current approach, especially it is not possible to define different grids.
I do like the idea of easy exchangeable grids, but I get the impression that this hack starts growing in complexity and when a hack is getting mroe complex, it requires reconsideration... perhaps there is a better solution to the problem.
I'd like to have solution, where you can easily change the grid but not by means of preprocessor magic. Perhaps it is possible to introduce this switch on the C++ level and thus allow different ways of changing the grid.

Up to now I don't have a better solution, but we need think about one. You imply that your changes and the centralization are an improvement. That is exactly what I doubt.

Christian

PS: no need to shout.

What bugs me is the automatic code generation. It adds a whole new level of complexity. Furthermore, the code generation is done by the AutoTools, which are not really intended for this purpose, and are notoriously difficult to understand and maintain. Of course much can be said in favour of automatic code generation. One could probably gain quite a lot of speed here and there. The FEniCS guys have shown how this can be done. But then you should do it right and not abuse the AutoTools.

Now, at least we have some hints why this hack is considered bad by some people. Let me try to straighten a few things:

There was a statement of the mailing list that this change needs discussion, which I quote here:

Hi Martin! Isn't this change something we should discuss and vote upon? I am not saying that it is bad, but I would like to have time to think about it and object. My first impression is that while creating gridtype.hh headers from m4 may(!) by a reasonable thing to, creating dgf-specific stuff is evil and I'd rather not have it.

Also, please don't insert lines like the one below without a comment line.

best, Oliver Till now, this discussion did not happen.

The preprocessor magic always required support from the build system to setup the automake variables. So we're only talking about the amount of m4 code. Moreover, if you don't want the magic, you don't have to let configure generate gridtype.hh / dgfgridtype.hh in your modules. It's entirely up to you.

There is no change in the requirement to support the preprocessor magic. Before the patch, a grid had to be entered into gridtype.hh by and to support the preprocessor magic (a quite tedious bit of work). Now, you just add one line to the m4 scripts. No grid developer has to add the grid, here.

@Oliver: Why are you worried about the maintance, we will do it (as before). What do you mean with doing it right? The generated code compiles and works as it should, and is still human readable. What more do you want? Maybe you have to get a bit more specific.

@Christian:

The main reason why I rejected the request for merge was, that it isn't working. I never asked for a merge. It seems to me that you didn't even look at the patch. The patch I send was to add a compatibility m4 script generating the headers gridtype.hh and dgfgridtype.hh witch will simply forward to the old locations. Nothing special here. I added a new patch only containing the m4 script, which will be sufficient for us. It will not affect the "stability" of the release in any way.

I just checked, the grid-howto works and I removed the last warnings. If the howto didn't work three weeks ago there might have been other reasons for not that.

Up to now the hack was limited to small part of the repository. That is still valid with the new solution, as you can see (still only two files, or old ones are deprecated). By the way I'd rather like to call it a solution to a problem than a hack. For a hack there are always better solutions available that have been avoided because of time issues. I don't see a better solution here, otherwise it would have been implemented.

It was always agreed that it isn't part of the interface. That is still valid with the new solution. Nobody is forced to use this.

But now after the latest changes all implementations have to provide their own part of the magic. ??? There is only one part for all grids. I don't understand why you think all implementations have to provide thier own part. It's one part for all.

There are certain things which are not possible with the current approach, especially it is not possible to define different grids. It was never intended to do that. You can come up with a solution that does it. Feel free. I think several people will be interested, including myself.

I do like the idea of easy exchangeable grids, but I get the impression that this hack starts growing in complexity and when a hack is getting mroe complex, it requires reconsideration Complexity was just decreased by puting all the good stuff to only one place. Thanks to Martin at this point.

perhaps there is a better solution to the problem. I'd like to have solution, where you can easily change the grid but not by means of preprocessor magic. Perhaps it is possible to introduce this switch on the C++ level and thus allow different ways of changing the grid. Until you have found such a solutiopn we can live with the current solution. This way you'll have time to think about this problem and we can carry on with our work. It's a win-win situation.

You imply that your changes and the centralization are an improvement. That is exactly what I doubt. If you read carefully my and Martins comments above there are reasons why we consider this changes to be an improvement. Up to now you have not explained why this is not the case (just mentioning it's not does not prove anything). The only thing you made pretty clear is that you don't like our solution. Can't help with that one, sorry. As you are not forced to use it, you'll get over it or invent something better. Again, a win-win situation.

Attachments

dune_gridtype.patch

I know you guys will maintain it, and maintain it well as usual. But what happens if you ever leave the business? Will new people be able to understand your code?

@Oliver: I think this is part of the game. If you would leave the "business" there will be parts of UGGrid nobody might understand immediately. If nobody uses this piece of code one day, it will be removed. Right now it is used a lot (Freiburg, Münster, ...) and therefore, is needed.

It seems to me that this discussion will lead nowhere. We should do what Oliver suggested in his first eMail about this: vote. If I understand this task correctly, there are two things to vote on: (a) should there be preprocessor magic in dune-grid? (b) if so, should it be generated by the autotools or should it be just the two headers in dune-grid?

As for the arguments on either point, I refer to the above discussion.

Obviously, my vote would be in favour of the generated files.

Like Martin, I also vote in favour of the generated files.

I vote in favor of the change

There seems to be pretty little interest in this vote. So far, we have 3 votes in favor of configure generated gridtype.hh 0 votes against configure generated files, but in favor of utility/gridtype.hh 0 votes completely against any form of gridtype.hh

We could, of course, try to deduce votes from the opinions given above, but that would be speculative.

I'll close this as voted in favor; if someone disagrees, feel free to reopen...

I vote against the new magic, as this will make gridtype.h part of the interface. For such a decision it too much a solution for an isolated problem.
I have no strong opinion regarding the gridtype.hh in general. I'm fine with a solution as a utility.

Sorry for not reacting earlier. I am actually waiting for Andreas' visit here next week. I would like to discuss the issue with somebody in person before I vote.

We discussed this topic during my stay in Berlin (Olil, Andreas and me).

As it turned out, the main concernes are:

change on the interface (different people consider this solution differently)
cluttering of the main directory of the module

Point 2 might me avoided if we manage to add these extra definitions to config.h. If I remeber correctly it is possible to add "plain text" to config.h. The gridtype/dgfgridtype.hh are some kind of config header, as they tell the system which grids are available. I think this would improve the situation a lot.

I still don't like the automatic code generation, but this would be good compromise.

An other aspect we should keep in mind, when adding intelligence to the build-system is the fact, that there are quite some groups out there using Dune with cmake. Thus my question... how do the discussed changes work effect the workflow of you, the average cmake-user?

Cheers Christian

I formally vote against the original proposal.

However the solution proposed by Christian is fine with me. It should give all features requested by Freiburg while being (to me) conceptually much more elegant. In particular

there are no new header files miraculously appearing out of nowhere (especially not in the main directory)
information about the build environment remains in config.h exclusively, following standard AutoTools procedure

I'm against putting that stuff into config.h. The reason is that this will require config.h to include some Grid's header files, which certainly means trouble. For instance, this means that every program of a given module will include the header for that grid since every program has to include config.h.

I'm not sure whether I like putting (dgf)gridtype.hh into the main directory, but it is certainly a better solution than putting that stuff into config.h.

I guess you have to do it right: all gridtype-related stuff needs to go at the end of config.h. Also, currently grid headers are only included if some additional preprocessor flags are set from the command line. Hence not every program is forced to include grid headers.

In revision 6731 of dune-grid I changed the m4 macro as suggested by Christian. Since the change is easy to revert, I just committed it directly to the trunk so that everybody can have a look.

Could the code generated in config.h please start with a meaningful introductory comment, so people who happen to look into the file know what it's about? Thanks.

I still see the lines

add support for GRIDTYPE=UGGRID to generated gridtype.hh / dgfgridtype.hh

DUNE_DEFINE_GRIDTYPE([UGGRID],[GRIDDIM == WORLDDIM],[Dune::UGGrid< dimgrid >],[dune/grid/uggrid.hh],[dune/grid/io/file/dgfparser/dgfug.hh])

in m4/ug.m4. Are they still needed the way they are? The comment is certainly wrong. Also, I find it strange that the test for a grid implementation should refer to a file reader.

Status changed to closed

Property	Value
Reported by	Martin Nolte (nolte@mathematik.uni-freiburg.de)
Reported at	Apr 12, 2010 11:23
Type	Feature Request
Version	Git (pre2.4) [autotools]
Operating System	Unspecified / All
Last edited by	Andreas Dedner (A.S.Dedner@warwick.ac.uk)
Last edited at	Aug 9, 2010 11:07
Closed by	Andreas Dedner (A.S.Dedner@warwick.ac.uk)
Closed at	Aug 9, 2010 11:07
Closed in version	2.1
Resolution	Implemented
Comment	gridtype definitions have been
added to config.h with
meaningful introductory comment

#771 configure generated gridtype.hh / dgfgridtype.hh

Metadata

Description

Designs

Child items ...

Activity

Attachments

add support for GRIDTYPE=UGGRID to generated gridtype.hh / dgfgridtype.hh