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

WIP: Panzer Owned and Ghosted GlobalEvaluationData Changes #1521

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

jmgate
Copy link
Contributor

@jmgate jmgate commented Jul 21, 2017

@eric-c-cyr and @rppawlo, we already did a code review of this refactor a while back, but changes pushed to Drekar required a refactor of the refactor (yay!).

The gist of it is Panzer's GlobalEvaluationData classes now have operator[]() and at() methods which take a local ID and give you the appropriate element of either the owned or ghosted vector. This way the user (in Panzer, Drekar, or elsewhere) doesn't need to know anything about the data being split out under the hood.

Should we ditch the at() routines? I included them for the sake of completeness and to mimmic the functionality of STL containers, but perhaps the bounds checking should happen in operator[]() surrounded by some #ifdef. Or maybe it already happens if -D Trilinos_ENABLE_DEBUG since they're Epetra_Vectors and Thyra::VectorBases behind the scenes—is that right?

In order to build and test this PR, you'll also need to pull the panzerGatherScatter branch from DrekarBase.

Note that I'll clean up the commit history of this branch before the merge to develop. This is just so we can talk it over.

@jmgate jmgate requested review from rppawlo and eric-c-cyr July 21, 2017 19:45
@jmgate jmgate added the stage: in progress Work on the issue has started label Jul 21, 2017
@jmgate
Copy link
Contributor Author

jmgate commented Jul 21, 2017

Including @ssmabuza on the discussion. He requests:

Is it possible to have the files with "Panzer_BasisIRLayout" modified to include this additional public data member:

Teuchos::RCP<PHX::DataLayout> local_mat_layout =
  Teuchos::rcp(new PHX::MDALayout<panzer::Cell, panzer::BASIS, panzer::BASIS>(
  this->numCells(), this->cardinality(), this->cardinality()));

This helps with creating local element matrices.

Sibu, can we talk this through in the meeting I set up for next Wednesday? I'm afraid I don't see right off the bat how this relates to this PR.

@eric-c-cyr
Copy link
Contributor

I would say add it to PureBasis, though I also don't see what it has to do with the PR.

@eric-c-cyr
Copy link
Contributor

@ssmabuza Do you have a patch for such a thing (a particularly design and set of members in mind?) This would not be hard to add.

@ssmabuza
Copy link
Contributor

ssmabuza commented Jul 21, 2017

@eric-c-cyr This is to provide the layout for PHX::MDField local matrix-like objects in the evaluators. Currently, we only have functional, basis and a few other layouts. At the moment I am bypassing this issue by creating in the constructor a pointer to PHX::DataLayout. local_mat_layout can be added to BasisIRLayout and in panzer::BasisIRLayout::setup(const panzer::PointRule & point_rule) we can simply add the line I sent to you above.

I'll send you a patch shortly.

@jmgate
Copy link
Contributor Author

jmgate commented Jul 27, 2017

Outcomes from yesterday's code review:

  • Move forward declarations into the panzer namespace—check that they're even needed.
  • Remove at() routines.
  • Move operator[] routines and associated data from derived classes into base classes to avoid virtual function calls.

@jmgate
Copy link
Contributor Author

jmgate commented Jul 27, 2017

@eric-c-cyr, @rppawlo: It looks like we have forward declarations outside the panzer namespace in more than just the code I've touched. Should I create an issue to deal with all those?

@jmgate
Copy link
Contributor Author

jmgate commented Jul 27, 2017

The issue with forward declarations will be handled for all of Panzer through #1548.

@jmgate
Copy link
Contributor Author

jmgate commented Jul 27, 2017

at() and associated routines removed.

@jmgate
Copy link
Contributor Author

jmgate commented Jul 27, 2017

Moved operator[]() routines and ownedView_ and ghostedView_ data to base classes. Testing now.

@jmgate
Copy link
Contributor Author

jmgate commented Jul 27, 2017

Tests pass. Rebasing on latest Trlinos/develop and DrekarBase/master.

The ReadOnlyVector_ and WriteVector_GlobalEvaluationData classes now
have Kokkos::Views of the owned and ghosted vectors.  Previously ghosted
vectors contained duplicates of all the information in the owned
vectors; now these two vectors are non-overlapping so we avoid data
duplication.  operator[]() routines were added to the
GlobalEvaluationData classes to hide the logic of element access from
users.  Previously one would call getGhostedVector() on the
GlobalEvaluationData and then index into that; now simply index into the
GlobalEvaluationData itself.

The LinearObjFactory classes were updated with "version 2" routines to
build these GlobalEvaluationDatas with non-overlapping owned and ghosted
vectors.  Eventually the original routines will be replaced by these new
ones, but not yet.

The Gather*Epetra* classes have been updated to use this new syntax when
working with GlobalEvaluationData containers.  When dealing with the
older EpetraLinearObjContainers, they still use the old way of doing
things.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants