idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.7k stars 1.04k forks source link

MooseMesh::nodeToElemMap() indexes all elements, including ancestors #6031

Closed dschwen closed 8 years ago

dschwen commented 8 years ago

MooseMesh::nodeToElemMap() builds a map that points from nodes to adjacent elements. It its current state it includes ancestor elements (i.e. elements that are parents holding refined elements). Is this desired behavior @friedmud ? This is tripping me up quite a bit in EBSD Reader.

if the lines

      MeshBase::const_element_iterator       el  = getMesh().elements_begin();
      const MeshBase::const_element_iterator end = getMesh().elements_end();

were changed to

      MeshBase::const_element_iterator       el  = getMesh().not_ancestor_elements_begin();
      const MeshBase::const_element_iterator end = getMesh().not_ancestor_elements_end();

we'd get a map that only holds real elements. I wonder if there is a usecase for having ancestor elements.

I know that I can filter these elements out by myself, but it makes my algorithm a lot less elegant (because I'll have to iterate over the element list twice to get the number of non-ancestor elements first).

FYI @frombs

dschwen commented 8 years ago

also quite a bit of that code in MooseMesh could be replaced with a call to MeshTools::build_nodes_to_elem_map (depending on what the decision on the iterator - with or without ancestors - is)

friedmud commented 8 years ago

Hmmm... I doubt that it's intentional to have all ancestors in there, but I might not be remembering all of the use cases. It is easy to filter yourself by just doing if(elem->active()) though... no need for two loops :-)

MeshTools::build_nodes_to_elem_map is NOT the same thing! Despite its name it creates an n_nodes length vector. Our stuff creates an actual map of only local (and ghosted) nodes... So storage is GREATLY reduced.

dschwen commented 8 years ago

Well, in #6032 I did already implement the filtering, and there you can see why it takes two loops. If you can figure it out in one loop I'll gladly implement it that way ;-)

dschwen commented 8 years ago

MeshTools::build_nodes_to_elem_map is NOT the same thing! Despite its name it creates an n_nodes length vector.

Right, I missed that part. This could be made into a function template though, as the code in the body is identical.

dschwen commented 8 years ago

And does it really GREATLY reduce storage? The vector is indexed by node ID. Aren't those pretty dense? Especially on serial meshes. And considering this map includes ancestors? Isn't there a substantial overhead on maps storage wise? Just making sure I'm not missing something here.

friedmud commented 8 years ago

It does greatly reduce the memory in the case of ParallelMesh... which is when we normally have hundreds of millions of elements/nodes so the size will matter ;-)

That said, I think we could definitely use a few different maps:

dschwen commented 8 years ago

I'll add NodeToActiveElemMap. Not local because I want ghost elements to be in that map.

dschwen commented 8 years ago

Ahhhhhhh! That's what SemiLocal is :-)

Yeah, of course, I'll change it to semilocal. Do I need to put semilocal into all the names though? Or can we just make this the conceptual default?

friedmud commented 8 years ago

Yes, put semilocal in the name.

friedmud commented 8 years ago

I'm actually hoping that once you get this in play we can try to switch everything over to using it... we should really try avoid making that global map :-)

dschwen commented 8 years ago

There is no active_semilocal_elements_begin. So either I add a new multipredicate to libmesh (why does that not exist? it seems useful) or I use semilocal_elements_begin and "manually" filter according to (*it)->active(). I guess I'll start with the latter.

friedmud commented 8 years ago

Or you could, you know, use this: https://github.com/idaholab/moose/blob/devel/framework/include/mesh/MooseMesh.h#L769

dschwen commented 8 years ago

But... I need an element range, not a node range...

jwpeterson commented 8 years ago

There is no active_semilocal_elements_begin. So either I add a new multipredicate to libmesh (why does that not exist? it seems useful)

Probably since no one needed it before now. It seems like a good thing to cache, because determining whether an Elem "semilocal" is kind of expensive, you have to call find_point_neighbors and iterate through them until you find one that shares your processor id.

permcody commented 8 years ago

Wait - you added new predicates to libMesh for getting ghosted elements. Can't we just create a semilocal range iterator in libMesh by combining ghosted and local predicates?

On Tue, Dec 8, 2015 at 4:17 PM John W. Peterson notifications@github.com wrote:

There is no active_semilocal_elements_begin. So either I add a new multipredicate to libmesh (why does that not exist? it seems useful)

Probably since no one needed it before now. It seems like a good thing to cache, because determining whether something an Elem "semilocal" is kind of expensive, you have to call find_point_neighbors and iterate through them until you find one that shares your processor id.

— Reply to this email directly or view it on GitHub https://github.com/idaholab/moose/issues/6031#issuecomment-163052927.

jwpeterson commented 8 years ago

Wait - you added new predicates to libMesh for getting ghosted elements. Can't we just create a semilocal range iterator in libMesh by combining ghosted and local predicates?

Well, he only wants active ones so it might not be the same exact thing. Anyway, no one is saying we don't know how to do it, just that it hasn't been needed till now.

dschwen commented 8 years ago

@permcody libmesh has a semilocal element iterator, it just does not have an active_semilocal iterator. b30f226 works around this by just doing a check for active() in the loop.