Iterate over GhostingFunctor* in vectors, not sets #4213
Conversation
It's entirely fair for GhostingFunctor subclasses to want to do parallel operations in e.g. mesh_reinit(), but we can't do that if they're not executed in the same order on all processors, and a set of pointers is definitely not guaranteed to be in the same order on all processors. I think I may have actually fixed this bug before anyone got bitten by it (while investigating other issues, though). This is technically an API change, but hopefully everybody uses "auto" for any ugly iterators that they have to declare, in which case this won't be an API change that breaks anyone's compile.
roystgnr
force-pushed
the
vector_gf_containers
branch
from
7ef74b8 to
d7a753c
Compare
|
I think I'm OK with the API change, however I would like to get a coupon for one free future API change of mine to be redeemed later 😆 Any idea if the MOOSE test failures here are real/actually due to this PR? |
|
BTW, would it make sense to put a public typedef on MeshBase like: This would give people something to use in cases where it's not possible to use |
|
The failures are definitely real - using If I can find all the other points where MOOSE was depending on that support, then I'll mark the revised support for it as deprecated for now, but if not then I'll just leave it supported indefinitely. Supporting it with vector instead of set gets us from O(log(N)) to O(N), but here N is the tiny number of ghosting functors and we're already unavoidably O(N*N_elem) when we actually query ghosting functors elsewhere. I thought about adding that typedef but decided to be lazy. Thanks for calling me on it; my superego now feels vindicated and my id chastened. |
src/base/dof_map.C
Outdated
| _coupling_functors.erase | ||
| (std::remove(_coupling_functors.begin(), | ||
| _coupling_functors.end(), | ||
| &coupling_functor)); |
There was a problem hiding this comment.
std::vector::erase(pos) erases only the element at pos... are you trying to erase all the elements that get std::removed here?
https://en.cppreference.com/w/cpp/container/vector/erase.html
roystgnr
force-pushed
the
vector_gf_containers
branch
from
54ae116 to
600f950
Compare
It's entirely fair for GhostingFunctor subclasses to want to do parallel operations in e.g. mesh_reinit(), but we can't do that if they're not executed in the same order on all processors, and a set of pointers is definitely not guaranteed to be in the same order on all processors.
I think I may have actually fixed this bug before anyone got bitten by it (while investigating other issues, though).
This is technically an API change, but hopefully everybody uses "auto" for any ugly iterators that they have to declare, in which case this won't be an API change that breaks anyone's compile.