#540 deprecate intersection iterators on entity
Metadata
| Property | Value |
|---|---|
| Reported by | Martin Nolte (nolte@mathematik.uni-freiburg.de) |
| Reported at | Apr 12, 2009 09:19 |
| Type | Feature Request |
| Version | Git (pre2.4) [autotools] |
| Operating System | Unspecified / All |
| Last edited by | Oliver Sander (oliver.sander@tu-dresden.de) |
| Last edited at | Jul 18, 2015 17:48 |
| Closed by | Oliver Sander (oliver.sander@tu-dresden.de) |
| Closed at | Jul 18, 2015 17:48 |
| Closed in version | Unknown |
| Resolution | Implemented |
| Comment | In the dune-grid 2.4 release, the methods ileafbegin/end and ilevelbegin/end have been completely removed from the Entity interface. |
Description
Now that the intersection iterators are provided by the grid views, they are no longer necessary on the entity. More precisely, the entity would be more slim without these methods.
So, should we deprecate the old interface on the intersection?
If so, what about hasBoundaryIntersections? Does it belong on the grid view?
Activity
Atgeirr Flø Rasmussen (atgeirr@sintef.no) commented on the issue:
Removing them from entities would remove them also from entity pointers and iterators. Wouldn't this make it harder to access intersections in generic algorithms?
A little example:
template void foo(EntityIter beg, EntityIter end) { for (EntityIter cur = beg; cur != end; ++cur) { auto ibeg = cur->ibegin(); // I know, auto is not widely available yet... auto iend = cur->iend(); ... // Code that works on intersections. } }
Above, foo() does not need the grid view. If only the grid view has ibegin(), we must now pass it as an argument to all such functions. Also, it is longer to write
gridview.ibegin(entity);than
entity.ibegin();without any (to me at least) obvious benefits.
Atgeirr
I don't see any advantage of removing these methods from the entity class. There are examples where one would like to use both methods and then would have to use both GridViews only to access what we already have now. The GridView IntersectionIterator access is only to assign a default IntersectionIterator, not to prohibit the other.
-
Atgeirr Flø Rasmussen (atgeirr@sintef.no) commented on the issue:
I would like to retain the intersection iterators (unless it's too late...). As far as I can tell, if all you have is a Grid g and a codim 0 Entity e, instead of e.ileafbegin() I would have to call g.leafView().ibegin(e). Won't this be inefficient? In some cases I could pass a view to my function instead (and for that case I agree that using the GridView::ibegin() method is an improvement), but if I also need to call Grid methods, I need to pass the grid and store/cache the view separately for efficiency reasons.
Atgeirr
PS. I get your point Christian, and realize the point I argued was mistaken. Thanks.
The const grid can be obtained from the GridView. I agree that in an adaptation marking strategy you need the non-const grid (to call mark) and might want to use the leaf intersection iterator to detect whether to mark (finite volume context). But additionally fetching the GridView does not cause any harm because you walk over the entire grid afterwards. And this grid walkthrough should be the dominant factor. Moreover, you might want to store the GridView in your discrete data anyway, because the data is useless without the index set.
I am undecided. On the one hand I am all for consistency and minimality, and that would mean to remove the method. On the other hand, getting an iterator for a container(-like thing) by a begin() method on that container is much more common c++/stl practice.
Therefore I explicitly abstain from the vote.
-
deprecating the methods was only the tiny part of this change. The problem is that all current grids use the DefaultGridView, which calls the suitable ibegin/ iend methods on the entity. To avoid spamming the user (i.e., me) with deprecation warnings, we are now forced to implement the grid views for all grids.
To have this in the release, I suggest to copy the DefaultGridView and only replace the methods ibegin / iend by the correct code.
It's like one a half years before. As far as I see, none of the grids, even the simpler ones like OneDGrid and YaspGrid, use DefaultGridViews. Implementing grid views for all grids seem too much work to me, regarding the gain of reducing the interface a little bit. If we want to reduce the interface, Christian's proposal ("We might leave the old GridView, make the ibegin / iend private and declare te DefaultGridView as friend.") seems more appropriate.
Because nobody really cares about this task, a change the "Due in Version" to undecided. Or should we declare this task won't fix?
-
I think a "won't fix" would send out the wrong signal to both, the users and the grid developers. I'd rather go with Christian's proposal and make the DefaultGridView call the methods on the entity implementation. This change requires one friend declaration if experimental-grid-extensions is disabled; otherwise no friend declaration is required. This way, we have at least a clean interface and newer grid implementations don't need to implement these methods anymore.
Just a side note: Don't blame the grid maintainers. There's very few of them and maintaining the grids does cause enough work without an extra implementation of the grid views.
-
I removed the methods from entity and made the DefaultGridViews friend of entity, see the attached patch. After a quick check I think YaspGrid, OneDGrid and SGrid work with this change. But I don't know what I should do with GeometryGrid because DefaultGridView calls Entity::i*() and this tries to call the same method from the host grid - but the host grid's method is gone. Is there a simple solution?
Attachments
-
@Christoph: No, without experimental grid extensions there is not. However I am prepared to fix this issue. If all the other grid implementations contained in dune-grid are working, I will be more than satisfied.
PS: Will the methods be deprecated or directly removed? If they are deprecated, GeometryGrid will at least continue to work even after your patch.
Some comments:
a) the old methods should get deprecated and not removed b) regarding GeometryGrid... wouldn't it be sufficient to forward via the hostgrid Level/LeafView? something like this: LeafIntersectionIterator ileafbegin () const { typedef GeoGrid::LeafIntersectionIterator< Grid > LeafIntersectionIteratorImpl;
-
return LeafIntersectionIteratorImpl( *this, hostEntity().ileafbegin() );
-
return LeafIntersectionIteratorImpl( *this, grid().hostGrid().leafView().ibegin(hostEntity()) ); }
c) instead of exposing the entity implementation class to the gridView, I'd add private methods _ileavebegin() _ileaveend() _ilevelbegin() _ilevelend() to the entity interface class.
Then we make sure that the gridview can access the methods (by making the grid view friend of the entity interface class), no matter how the authors implemented a specific entity.
-
-
a) Sure, I'll deprecate the methods. But for testing removing shows the shortcomings better. b) I only tested it with test-geogrid but that compiled and run through. I choose the level in hostGrid().levelView 0, right? c) I'll do that and then check what needs to be done and how the three big external grids behave. Thanks for your comments.
-
I implemented the suggestions, cf. the attached patch. Make check run through for all included grid. ALUGrid passed, too. Alberta just failed on the same tests as it does for me the last half year. Only UG code could not be compiled. One of the reasons is uggrid.cc line 645 ff, there is the LevelIntersectionIterator but no grid view near.
I'll run headercheck within the next days.
Attachments
-
@Christian: Understandable. However, I did not want to learn the coding style used in UGGrid.
Basically, I did two things:
- Use the grid view to access the iterators / intersection iterators
- Fetch some objects for later use (e.g., don't call *it or intersection.outside() more than once)
The rest is white space changes so that I can read the code. Feel free to reduce them to a minimum when merging.
-
- changes for test-oned are already commited
- Martin, you changed a lot in GeometryGrid. Probably my solution is inferior to yours, but I hesitate to merge them because it's not clear what is only for testing and what should go to trunk.
- I'll have a look for your UG changes and will try to merge without unnecessary whitespace changes
- Martin, you choose another way of changing default grid view and entity. That means merging r8539 and r8555 to trunk (with deprecating instead of removing). Or did I miss a commit?
-
@Christoph: The commits I listed above should do the trick. Patch 8539 should not be required, but might be nice to have anyway. However, they might not merge without some minor manual fixes (the branches differ slightly by now).
I will merge the changes to GeometryGrid myself (as soon as possible); they natively the implement GridViews and improve the overall structure of GeometryGrid.
As to the other patches: I will not merge the patch for UGGrid, since it seems to require some polish. The interface patch can be done by anyone (though I will do it if necessary).
