idaholab / moose

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

isSemiLocal != isGhosted #7355

Open permcody opened 8 years ago

permcody commented 8 years ago

Description of the enhancement or error report

As we've discussed several times in the recent past, SemiLocal != Ghosted. This is actually a big problem because we use isSemiLocal() throughout the framework to "safely" access solution values all the time. This really needs to be fixed and I'm not sure how to do it efficiently. I was able to build my own ghosted element structure by looping over all elements and looking for neighbor processor id mismatches but there needs/has to be a better way.

Take a look at our usage: https://github.com/idaholab/moose/blob/devel/framework/src/base/MooseVariable.C#L1609-L1650

Rationale for the enhancement or information for reproducing the error

This is a ticking time bomb

Identified impact

(i.e. Internal object changes, limited interface changes, public API change, or a list of specific applications impacted)

Everything

Tag @idaholab/moose-developers

friedmud commented 8 years ago

I would like this fixed in libMesh. See here: https://github.com/libMesh/libmesh/issues/977#issuecomment-222787195

However, the MOOSE meaning of "semilocal" doesn't match directly with the libMesh meaning. Our meaning of it in MooseMesh does guarantee that the solution values exist on the local processor. You can see how we build that list here:

https://github.com/idaholab/moose/blob/devel/framework/src/mesh/MooseMesh.C#L519

In MooseMesh "semilocal" means: the element is either local or it's ghosted.

So there is no bug here... and it's NOT a defect (we would have hit it a LONG time ago if that were the case).

friedmud commented 8 years ago

I've reorganized this as a task... because it probably isn't a good idea to have this mismatch between meanings of semilocal. But, like I say, I really want this fixed in libMesh.

I want libMesh to act like MooseMesh does: semilocal means active + ghosted

permcody commented 8 years ago

OK - well that would explain a few things. I'm not saying I've had problems with MOOSE's semi local meaning. I did run into a case (3D with at least two levels of refinement) where libMesh's semi local returned true when it wasn't ghosted... You may have seen Roy's response to that, it was complicated as all can be without any resolution.

Let's discuss this at the tiger team, if Roy doesn't want to change his meaning (which he may not) then perhaps we should rename our meaning. The mismatch is bad.

permcody commented 8 years ago

Tagging @roystgnr

roystgnr commented 8 years ago

On the one hand, IIRC I invented the term "semilocal", and if you guys wanted to know what I meant by it you should have asked, and if you wanted to refer to a different concept then you should have invented a different word! ;-)

On the other hand, there are probably external users of the "moose-semilocal" concept and there are probably only internal uses of the "libMesh-semilocal" concept, so if we want to minimize confusion then we probably ought to rename the latter, not the former. However, what would you call an element on a distributed mesh which has to exist on a processor it's not owned by but whose nodal or elemental DoFs don't have to?

permcody commented 8 years ago

For the record, I'm not stuck on the name for either concept 😄. I don't really care which one we rename. I'll just be happy when I'm no longer confused.

jwpeterson commented 8 years ago

However, what would you call an element on a distributed mesh which has to exist on a processor it's not owned by but whose nodal or elemental DoFs don't have to?

The meaning of "semi-local" element in libmesh seems to be intimately related to the concept of point neighbors (see below). That is, if any one of my point neighbors has processor id == X, I am semi-local to processor X. So if we are renaming things, I think the new name should involve point neighbors, e.g. Elem::has_point_neighbor_with_pid(processor_id_type X).

bool Elem::is_semilocal(const processor_id_type my_pid) const
{
  std::set<const Elem *> point_neighbors;

  this->find_point_neighbors(point_neighbors);

  std::set<const Elem *>::const_iterator       it  = point_neighbors.begin();
  const std::set<const Elem *>::const_iterator end = point_neighbors.end();

  for (; it != end; ++it)
    {
      const Elem * elem = *it;
      if (elem->processor_id() == my_pid)
        return true;
    }

  return false;
}
roystgnr commented 8 years ago

Yeah; originally the idea was pure ParallelMesh: a "ghost" element is an element we don't own but we need to keep a copy of, a "semilocal" element is either a local or a ghost element.

But then you get to the confusing bit: a ghost element may (because it's a point neighbor but not a side neighbor, or because it's a parent of a side neighbor, whatever) have degrees of freedom which themselves don't need to be ghosted.

roystgnr commented 8 years ago

The trouble with point-neighbor-related names is that I wouldn't swear we need to ghost point neighbors. We are ghosting point neighbors, and I can think of a few reasons why that's useful, but I don't recall a reason why it was absolutely necessary, and if there is none then it's a behavior we may want to change in the future, at which point we'd need another new name for ghosted-or-local elements.

roystgnr commented 8 years ago

Oh, and more importantly, once we get Derek's "you ghost what I tell you to ghost" API solid, then we'll potentially have ghosted elements which aren't guaranteed to be point neighbors, and we'll need the "semilocal" or "formerly_known_as_semilocal" name to refer to those too.

jwpeterson commented 8 years ago

We are ghosting point neighbors,

I don't think we are, that's what led @permcody down this whole road in the first place... he thought he could get the dofs of any semi-local element, but that was not the case.

roystgnr commented 8 years ago

The problem isn't that we aren't, it's that we are. We are ghosting point neighbors; we aren't ghosting point neighbors' dofs.

jwpeterson commented 8 years ago

The problem isn't that we aren't, it's that we are. We are ghosting point neighbors; we aren't ghosting point neighbors' dofs.

OK, sorry, should have been more specific about elems vs. dofs.

friedmud commented 8 years ago

I'm currently thinking that we leave semilocal along in libMesh. Can we come up with a better name for all of the elements that exist locally AND have all of their degrees of freedom ghosted locally? This is what I would like to define as an element being "ghosted": it exists in the local Mesh and all of its dofs are locally ghosted.

Maybe locally_available? Hmmm... reminds me too much of emotionally_available ;-)

We really could just call the iterator local_and_ghosted_active_elements and just be explicit about it.

permcody commented 8 years ago

Well if your your dof isn't locally_available (emotionally_available) and you try to communicate with that dof, it'll wreck your whole simulation... So that little play on words isn't really that far off.

I don't think locally_available is too bad. I was going to suggest just available which is more succinct and just as clear.

Here are a few other options

jwpeterson commented 8 years ago

Here are a few other options

  • can_haz_value
roystgnr commented 8 years ago

"semilocal" is supposed to be a synonym for "local_and_ghosted". The trouble is the distinction between ghosted elements and ghosted dofs.

"accessible" and "usable" are good for the "all dofs are semilocal too" case. But hell, I actually seriously like "can_haz_value" more than either of those - it's the first suggestion that really explains, "you can access solution values on this element". "evaluatable", maybe?

permcody commented 8 years ago

I guess I can't think of very many places where it's useful to ghost the element (only affects parallel mesh) but NOT the dof. Is this useful in practice?

roystgnr commented 8 years ago

Yeah; e.g. we've got some raycasting code which needs serial geometry (to figure out where the rays go) but then can do parallel calculations along those rays once each processor knows who its talking to.

permcody commented 8 years ago

Ah - Perfect! That's what I wanted to know.

roystgnr commented 8 years ago

We're definitely going to want separate APIs for "mark this element for ghosting" vs. "mark this element for evaluating", though, and I'll bet the latter gets more use than the former.

roystgnr commented 8 years ago

@permcody and @friedmud, could you check out my remaining design issues in https://github.com/libMesh/libmesh/issues/1028#issuecomment-236750707 and let me know where they fall on a scale of "not a problem" to "annoying" to "showstopper"?

Summary: with the API I'm proposing, we still won't be able to do Elem::is_semilocal() or Elem::is_evaluable() queries, because there's no way to make those queries O(1) without batch-precomputing and caching them. However, you seem to already be precomputing and caching _semilocal_node_list in MOOSE, and we will be able to provide Mesh::list_ghosted_elements(vector&) and DofMap::list_evaluable_elements(const Mesh&, vector&) (or list_foo_nodes) methods to do that precomputation for you.

permcody commented 8 years ago

I'll add a few comments on the libMesh side but I'll comment on the specific summary here since it's relevant to MOOSE. As far as I know we actually don't need is_semilocal (Roy's definition) in MOOSE. As Derek clarified earlier, MOOSE's isSemilocal is actually equivalent to the proposed Elem::is_evaluable(). Also, we are caching elements in MOOSE because libMesh does not. I would love to see all of this element/node caching and manipulation move out of MOOSE.