NumericVector::is_effectively_serial(), to avoid overloading metadata#4428
Conversation
Shorthand to make it easier to determine whether a vector can be handled using optimized-for-serial code paths regardless of its conceptual status.
This ought to preserve the performance improvements of Seq vectors and serial codepaths without losing PARALLEL-vs-GHOSTED metadata.
|
Thanks, this fixes the original test that was failing for me, I'm going to temporarily push your branch into upstream so that I can run all of our internal testing on it as well. |
|
Huh, looks like maybe only one MOOSE test doesn't like this. |
|
It's so insulting when just one test fails. "Hey, your code sucks and your test coverage sucks!" I can reproduce this but probably won't find time to fix it before tomorrow. |
And even in the serial case our metadata should reflect that. This fixes a conflict with MOOSE restarts (which want to save a subvector in one cases but can't save a ghosted vector) for me.
|
@lindsayad won't have time to check this for performance regressions for a while, but it's at least fixing correctness regressions so he's good with us merging now. |
| // You'd think we would create GHOSTED from GHOSTED, but we | ||
| // never have, and now there'd be downstream compatibility | ||
| // issues if we started even marking an effectively-serial | ||
| // subvector as GHOSTED. |
There was a problem hiding this comment.
I guess I could have changed the libmesh_error_msg_if on line 1058 to libmesh_not_implemented() to maybe avoid your ire here? I had thought about this and decided to not implement support (yet) and hence created the error
There was a problem hiding this comment.
Well, the catch isn't a problem with the incoming type of the subvector that we pass in and assert about, it's on the type of this.
In hindsight we probably should have been looking at both types: if the subvector has a type() other than AUTOMATIC, respect that; if it has AUTOMATIC, determine its new type() from this->type(). And then at that point it would be reasonable to libmesh_not_implemented() on the this->type() == GHOSTED, subvector.type() == AUTOMATIC and the subvector.type() == GHOSTED cases.
Maybe that's not too late to do, actually, if you think it's a reasonable idea? We could patch downstream to use that mechanism to request the GHOSTED->PARALLEL case that we now do implicitly, then patch here to respect that mechanism afterward.
No ire in your direction, BTW, just frustration at the situation. The first version of create_subvector was from John in 2004, and didn't consider type() at all. We have more than a few code design decisions from circa 2004-2007 (including my own) that have caused us backwards compatibility hassle but that I find hard to criticize anyway. 2004 libMesh is like that Russian quip: "The marvel is not how well the dancing bear dances, but that it dances at all".
4 participants
Hopefully this will fix the regressions @jwpeterson saw in write-1-to-read-N after #4374