Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dschwen opened this issue Dec 7, 2015 · 17 comments
Closed

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

dschwen opened this issue Dec 7, 2015 · 17 comments
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: task An enhancement to the software.

Comments

@dschwen
Copy link
Member

dschwen commented Dec 7, 2015

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
Copy link
Member Author

dschwen commented Dec 7, 2015

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
Copy link
Contributor

friedmud commented Dec 8, 2015

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
Copy link
Member Author

dschwen commented Dec 8, 2015

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
Copy link
Member Author

dschwen commented Dec 8, 2015

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
Copy link
Member Author

dschwen commented Dec 8, 2015

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
Copy link
Contributor

friedmud commented Dec 8, 2015

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:

  • NodeToElemMap (what it is now)
  • ActiveLocalNodeToElemMap
  • ActiveSemiLocalNodeToElemMap

@dschwen
Copy link
Member Author

dschwen commented Dec 8, 2015

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

@dschwen
Copy link
Member Author

dschwen commented Dec 8, 2015

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
Copy link
Contributor

friedmud commented Dec 8, 2015

Yes, put semilocal in the name.

@friedmud
Copy link
Contributor

friedmud commented Dec 8, 2015

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
Copy link
Member Author

dschwen commented Dec 8, 2015

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
Copy link
Contributor

friedmud commented Dec 8, 2015

@dschwen
Copy link
Member Author

dschwen commented Dec 8, 2015

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

dschwen added a commit to dschwen/moose that referenced this issue Dec 8, 2015
@jwpeterson
Copy link
Member

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
Copy link
Member

permcody commented Dec 8, 2015

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
#6031 (comment).

@jwpeterson
Copy link
Member

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 added a commit to dschwen/moose that referenced this issue Dec 8, 2015
@dschwen
Copy link
Member Author

dschwen commented Dec 8, 2015

@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.

@permcody permcody added C: Framework T: task An enhancement to the software. P: normal A defect affecting operation with a low possibility of significantly affects. labels Dec 9, 2015
dschwen added a commit to dschwen/moose that referenced this issue Dec 9, 2015
dschwen added a commit to dschwen/moose that referenced this issue Dec 9, 2015
@dschwen dschwen closed this as completed Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Framework P: normal A defect affecting operation with a low possibility of significantly affects. T: task An enhancement to the software.
Projects
None yet
Development

No branches or pull requests

4 participants