Interior and Exterior Facets for Extruded Meshes #334
Conversation
| @@ -2493,6 +2493,7 @@ def __init__(self, iterset, toset, arity, values=None, name=None, offset=None, p | |||
| # the application of strong boundary conditions | |||
| self._bottom_mask = np.zeros(len(offset)) if offset is not None else [] | |||
| self._top_mask = np.zeros(len(offset)) if offset is not None else [] | |||
| #self._int_facet = int_facet | |||
|
You've added a bunch of stuff to the /public/ API, but have not documented it properly. Either with full docstrings, or with appropriate sections in the manual. Can you do, at the very least, the former please? |
|
How does this look now? |
|
I think this is mostly fine. Though I don't really like the way the sparsity builder gets more and more cluttered. Sure, we don't want to have branch in inner loops, but I think this could be refactored with inline functions, which are supported by cython. |
Add the iteration descriptor to the loop cache key.
| @@ -2550,6 +2550,11 @@ def split(self): | |||
| return (self,) | |||
|
|
|||
| @property | |||
| def iteration_region(self): | |||
| """:class:`Set` mapped from.""" | |||
| return [ALL] | |||
There was a problem hiding this comment.
This really isn't a Set. It's also sort of an internal property. Does the user ever need to see this, and if not, can it be an internal property?
|
My comments at this level are mostly issues about documentation. In addition to docstrings, bits of the manual need updating as well, in particular, probably the section on building sparsities. On a more general note, the API for extruded objects in PyOP2 is getting quite hairy, as we add things to it piecemeal when we realise we need them. As a result, it's becoming somewhat difficult to see what's an implementation hack and what is a nice API abstraction. The API for non-extruded stuff went through a number of iterations of polishing to nail down what it should look like. I think the extruded API probably needs some design loving too before we try and add much more to it. |
| @@ -3239,6 +3276,44 @@ def _dump_generated_code(self, src, ext=None): | |||
| f.write(src) | |||
|
|
|||
|
|
|||
| class IteratationRegion(object): | |||
|
I agree with @wence-'s objection that the API for extrusion needs some more thinking. It feels more and more like hack on hack and doesn't quite fit the otherwise nicely designed abstraction. In particular since you're now talking about facets, which is not completely abstraction breaking but a feature of PyOP2 is that it doesn't care what entities represent. |
|
I agree that it's not the pretties looking API but you make it sound worse than it is. I think we have to accept the fact that until we implement something we don't know what the best way to do it will be and it is a natural stage for any API to not be pretty right from the very beginning. I think at the present time it does the job quite nicely as I think we've already avoided considerably worse looking versions of this API. It would also help to be more specific about what features of this API we would like to change. Also, I think people need to use it first so that they realise what they really want. |
|
Sure, I agree with that. However, from a high level, it seems like most of the extra pieces in the API are about telling the generated code about which bits of the implicit Map it should build, or which bits of an extruded set it should iterate over. I wonder if there is a nicer way of expressing this than currently, although, as you say, I have not used things in anger yet. |
| {'name': self.c_map_name(i, j), | ||
| 'dim': m.arity, | ||
| 'ind': idx, | ||
| 'dat_dim': str(cdim), | ||
| 'ind_flat': str(m.arity * k + idx), | ||
| 'offset': ' + '+str(k) if k > 0 else ''}) | ||
| 'offset': ' + '+str(k) if k > 0 else '', | ||
| 'off_top': ' + '+str((layers - 2) * m.offset[idx]) if is_top else ''}) |
There was a problem hiding this comment.
This makes the generated code specific to the number of layers again, is that right?
|
This still has a few outstanding issues to be resolved, but is nearly ready to go I think. I will reword the docstrings a bit to remove use "now accepts" once everything else is fixed. |
Implement iteration over regions of extruded sets. Necessary for forming interior and exterior facet integrals in the extruded case.
|
And we're in. |
Implements interior and exterior facets for Extruded Meshes.