dolfin team mailing list archive

[Anders Logg <logg@xxxxxxxxx>]

On Wed, Oct 29, 2008 at 04:10:52PM +0100, Niclas Jansson wrote:
> Anders Logg wrote:
>> On Mon, Oct 20, 2008 at 03:57:29PM +0200, Niclas Jansson wrote:
>>> Anders Logg wrote:
>>>> On Fri, Sep 26, 2008 at 05:35:56PM +0200, Niclas Jansson wrote:
>>>>> Anders Logg wrote:
>>>>>> On Fri, Sep 26, 2008 at 11:35:38AM +0200, Niclas Jansson wrote:
>>>>>>> Anders Logg wrote:
>>>>>>>> On Mon, Sep 22, 2008 at 06:17:49PM +0200, Anders Logg wrote:
>>>>>>>>> On Mon, Sep 22, 2008 at 06:13:34PM +0200, Niclas Jansson wrote:
>>>>>>>>>> Anders Logg wrote:
>>>>>>>>>>> On Mon, Sep 22, 2008 at 05:03:59PM +0200, Niclas Jansson wrote:
>>>>>>>>>>>> Anders Logg wrote:
>>>>>>>>>>>>> I'm slowly working my way through PXMLMesh.cpp and I need some
>>>>>>>>>>>>> assistance (from Niclas).
>>>>>>>>>>>>>
>>>>>>>>>>>>> It all looks very good and I'm sure it works perfectly, but some work
>>>>>>>>>>>>> is needed to make it possible to read/understand:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. PXMLMesh::closeMesh really needs some commenting. It's a large
>>>>>>>>>>>>> chunk of nontrivial and quite complex code so a whole lot of
>>>>>>>>>>>>> commenting is needed.
>>>>>>>>>>>> I'm working on it.
>>>>>>>>>>> ok, nice.
>>>>>>>>>>>
>>>>>>>>>>>>> 2. Why is the mesh editor closed in PXMLMesh::readCells and then
>>>>>>>>>>>>> opened again later in PXMLMesh::closeMesh?
>>>>>>>>>>>>>
>>>>>>>>>>>>> If we can't write to the mesh until everything is read in, then maybe
>>>>>>>>>>>>> we should wait with initializing anything until closeMesh() and at
>>>>>>>>>>>>> that point start editing the mesh?
>>>>>>>>>>>>>
>>>>>>>>>>>> It's a (ugly) workaround due to the static nature of the MeshEditor
>>>>>>>>>>>> (number of vertices must be specified a priori). It could of course be
>>>>>>>>>>>> fixed by moving the geometric partitioner inside the parser, and only
>>>>>>>>>>>> opening the editor inside closeMesh().
>>>>>>>>>>>>
>>>>>>>>>>>> However the nice solution would be to make the MeshEditor more dynamic.
>>>>>>>>>>>> That would also make life easier when implementing a better refinement
>>>>>>>>>>>> algorithm (for example, the recursive longest edge bisection (Rivara)
>>>>>>>>>>>> from unicorn)
>>>>>>>>>>> I agree it would be better to make MeshEditor dynamic. The problem is
>>>>>>>>>>> that this is not always needed. Perhaps we should add a new class
>>>>>>>>>>> DynamicMeshEditor that can be used when one does not know the number
>>>>>>>>>>> of vertices and cells a priori.
>>>>>>>>>>>
>>>>>>>>>>> It could be very simple, just storing the dynamic data in suitable STL
>>>>>>>>>>> containers and then calling MeshEditor in close().
>>>>>>>>>>>
>>>>>>>>>>>>> 3. I don't understand the logic in readTriangle/readTetrahedron.
>>>>>>>>>>>>> What does the following code do?
>>>>>>>>>>>>>
>>>>>>>>>>>>>   if (!(is_local(v2) || is_local(v1) || is_local(v0)) || !is_local(v0))
>>>>>>>>>>>>>     return;
>>>>>>>>>>>>>
>>>>>>>>>>>>>   used_vertex.insert(v0);
>>>>>>>>>>>>>   if (is_local(v1))
>>>>>>>>>>>>>     used_vertex.insert(v1);
>>>>>>>>>>>>>   if (is_local(v2))
>>>>>>>>>>>>>     used_vertex.insert(v2);
>>>>>>>>>>>>>
>>>>>>>>>>>>>   if (!(is_local(v1) && is_local(v2) && is_local(v0)))
>>>>>>>>>>>>>   {
>>>>>>>>>>>>>     if (!is_local(v1))
>>>>>>>>>>>>>       shared_vertex.insert(v1);
>>>>>>>>>>>>>     if (!is_local(v2))
>>>>>>>>>>>>>       shared_vertex.insert(v2);
>>>>>>>>>>>>>   }
>>>>>>>>>>>>>
>>>>>>>>>>>> The idea is to assign the triangle/tetrahedron to the processor who owns
>>>>>>>>>>>>   vertex v0. And, yes the logical could probably be more clearer.
>>>>>>>>>>> ok, I see.
>>>>>>>>>>>
>>>>>>>>>>> It seems that
>>>>>>>>>>>
>>>>>>>>>>>   !(is_local(v2) || is_local(v1) || is_local(v0)) || !is_local(v0)
>>>>>>>>>>>
>>>>>>>>>>> is equivalent to
>>>>>>>>>>>
>>>>>>>>>>>   (!is_local(v2) && !is_local(v1) && !is_local(v0)) || !is_local(v0)
>>>>>>>>>>>
>>>>>>>>>>> which is equivalent to
>>>>>>>>>>>
>>>>>>>>>>>   !is_local(v0)
>>>>>>>>>>>
>>>>>>>>>>> Is this correct, and is this what you have in mind?
>>>>>>>>>> It looks correct to me. Seems like I should refresh my logic, that was a
>>>>>>>>>> really simple reduction :)
>>>>>>>>>>
>>>>>>>>>> Niclas
>>>>>>>>> It wasn't very obvious that it could be so much simplified, but it
>>>>>>>>> should help to clear up the logic.
>>>>>>>>>
>>>>>>>>> I'll wait for you to do some more work on PXMLMesh.cpp before I
>>>>>>>>> continue reading it.
>>>>>>>>>
>>>>>>>>> If possible, I'll add DynamicMeshEditor later tonight so you can use
>>>>>>>>> that to simplify PXMLMesh.cpp further.
>>>>>>>> I've added DynamicMeshEditor. Try to use this in PXMLMesh.cpp. It may
>>>>>>>> simplify it a bit.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Commenting and bug fixes in PXMLMesh.
>>>>>> Thanks, it looks much better! I'll make a new attempt to get through
>>>>>> the code (after the suggested changes below).
>>>>>>
>>>>>>> I couldn't use DynamicMeshEditor to get rid of the opening and closing
>>>>>>> of the mesh editor. The problem is that each processor parse (and
>>>>>>> receive from partitioning) vertices which are not used in the local
>>>>>>> cells. These must be removed before the final mesh is returned, and this
>>>>>>> is not supported by the editor.
>>>>>>>
>>>>>>> I think the easiest solution to this problem is to move the geometric
>>>>>>> partitioner inside the XML parser. Since it's produces rather bad
>>>>>>> partitions I don't see why anyone would like to use it in an application.
>>>>>> That sounds good, especially since you now call the partitionGeom()
>>>>>> before the mesh is even a mesh (it has only vertices).
>>>>>>
>>>>>> Can you make an attempt to do the geometric partitioning inside
>>>>>> PXMLMesh?
>>>>>>
>>>>>> I also suggest that you don't open the mesh editor until you get to
>>>>>> the closemMesh() function. Just store the vertices in an Array<>
>>>>>> (which is the same as an std::vector). You don't call editor.addCell()
>>>>>> until closeMesh() so it's not really a Mesh before then anyway.
>>>>>>
>>>>>> It also fits well with how you treat the cells. You already have a
>>>>>> cell_buffer. You could store a vertex_buffer similarly.
>>>>>>
>>>>>> So it would be something like this:
>>>>>>
>>>>>> 1. Read vertex coordinates into a simple Array (or similar).
>>>>>>
>>>>>> 2. Partition the vertices.
>>>>>>
>>>>>> 3. Build the Mesh in closeMesh() using MeshEditor.
>>>>>>
>>>>>> Would that work?
>>>>>>
>>>>> I think so. I'll try fix this and insert the geometric 
>>>>> partitioner as soon as possible.
>>>>>
>>>>> Niclas
>>>> Any progress on the patch?
>>>>
>>>>
>>> No. I'll probably have time to fix it this week.
>>>
>>> Niclas
>>
>> ok.
>>
>>
>
> Done, moved partitioner into PXMLMesh and cleaned up closeMesh().
>
> Niclas

Thanks! It looks good at first glance. I'll take a closer look.

It's been pushed.

-- 
Anders

Attachment: signature.asc
Description: Digital signature