Privatize FEMContext #108

Merged
merged 20 commits into from Jul 2, 2013

4 participants

@pbauman
libMesh - C++ Finite Element Library member

This pull request contains the (I believe...) final changes needed to move some of the data structures to protected/private status and provide accessors for them. The original impetus for this was dealing with vector-valued finite elements. Accordingly, the cached Elem* is now accessible through get_element_fe (get_side_fe, get_edge_fe, respectively). Other major changes include quadrature accessors return a reference instead of a pointer (adhering to the rule-of-thumb if one needs to check for nullness, use a pointer; otherwise use a reference), and provide per-variable access to dof_indices, residuals, and Jacobians. I left the side and edge objects public because, it seems, they're used as iterators in places and it just seemed easier to leave them public.

As I got farther along with this, I decided I don't really like the get_* API naming; seems kind of sophomoric. I left it, but I'd be open to going through and sed'ing out a lot of those. I'm open to suggestion.

The other thing I found annoying was have to provide non-const references to some of the objects to either set values or pass along to other functions. I'm open to suggestion there.

@roystgnr, please have a careful look. I'm happy to make any changes. @dknez, I tried to keep the reduced basis changes localized into a couple of commits. Please let me know if I FUBAR'ed anything.

@roystgnr
libMesh - C++ Finite Element Library member

Regarding get_foo(), the options I've seen are:

1) Foo foo; // no setters/getters
2) Foo get_foo() const (or const Foo& get_foo() const); void set_foo(const Foo& f).
3) Foo foo() const (or const Foo& foo() const); void set_foo(const Foo& f).
4) Foo foo() const (or const Foo& foo() const); Foo& foo().
5) Property foo;

It's really cool that option 5 is possible, but it uses magic which could be misleading to advanced C++ users and baffling to novices; in fact the best implementation I could quickly google is described by the author as "it's hideous". http://www.cplusplus.com/forum/general/8147/ Its single (huge) advantage is that it makes "start with option 1, then upgrade to 5 if needed" an API-compatibility-preserving strategy.

I think option 2 is the "Java way"; identical to the "Objective C way" in option 3 but more verbose.

Option 4 would probably be the "C++ way", except that people noticed that the name overloading makes using std::bind or any other pointer-to-method based code an unnecessary ugly hassle.

I think we've got a mix of options 1, 2 and 4 in the library now. I'd lean towards option 3, but my real preference would be to hash this out, just settle on one style (counting "1, replaced by 5 if/when needed" as a single style) and then eventually deprecate and replace the others.

I'll try to check through your patch ASAP; thanks!

@dknez
libMesh - C++ Finite Element Library member

The changes to the RB examples look good to me. (I haven't pulled and tested myself yet though, I'll wait in case there are any more API changes before I update all my local code.)

One question re 7b8d013: where does the RB stuff require a non-const accessor for dof_indices? That's probably just laziness on my part, and perhaps it should be fixed in the RB code.

@pbauman
libMesh - C++ Finite Element Library member

@dknez: Sorry, that was a poor commit message. The constrain_element_matrix_and_vector, etc. calls within RB need non-const dof_indices.

@pbauman
libMesh - C++ Finite Element Library member

I should say that change ended up actually happening in b13e022

@dknez
libMesh - C++ Finite Element Library member

OK, makes sense, thanks for clarifying!

@dknez
libMesh - C++ Finite Element Library member

Paul, I pulled your branch. The RB stuff looks good, but RB examples 2 and 7 don't compile. Those require --enable-slepc and --enable-complex, respectively, which is probably why.

I'll fix up these two examples and push to my fork (dknez/libmesh). I'm guessing you can then update your pull request by pulling from my fork?

@pbauman
libMesh - C++ Finite Element Library member

Arg! I always forget to check --enable-complex. Sorry! I'll pull from your fork and apply to the pull request. Thanks!

@benkirk
libMesh - C++ Finite Element Library member
@dknez
libMesh - C++ Finite Element Library member

These examples work in my fork now (master branch), so feel free to pull!

@pbauman
libMesh - C++ Finite Element Library member

OK, I've integrated @dknez's changes (sorry for my git noobishness, wasn't sure how to merge across repos so I just applied the patches and listed @dknez in the commit log).

@dknez
libMesh - C++ Finite Element Library member

I'd be happy for this to be merged so that my colleagues can use the new API easily. Any objections, or are we good to go?

@pbauman
libMesh - C++ Finite Element Library member

@dknez: I'm good to go if you're OK with the API. I think Roy is going to be looking at it in the very near future, so I was going to let him hit the button if he was happy.

@dknez
libMesh - C++ Finite Element Library member

I'm happy with the API. Also, FWIW, I quite like the get_* naming.

If you want to wait for Roy, that's of course fine by me.

@roystgnr
libMesh - C++ Finite Element Library member

This looks good to go in the long term, but I'm not sure this is quite what should go into 0.9.2 final. We've got a couple didn't-exist-before-this-patch and different-signature-before-this-patch accessors here, and we normally wouldn't be making the stuff they access private until a release after the release of the new accessors, to make it possible to write application code that's more backwards compatible. Failure to do that in the past has made bisecting regression failures in apps more painful than necessary for me.

But if Paul and David are both happy as-is, I'll consider myself outvoted. Either of you feel free to push the button. ;-)

@pbauman
libMesh - C++ Finite Element Library member

Hmm, I see your point. 248f1fa contains only the "making everything private" part so we could revert that commit for the 0.9.2 release and then reapply after the release. This sounds like a much more sane idea.

@roystgnr
libMesh - C++ Finite Element Library member

Great!

@dknez
libMesh - C++ Finite Element Library member

Sounds good to me!

pbauman added some commits Jun 6, 2013
@pbauman pbauman Rb stuff needs a non-const reference to dof_indices. fac9b2b
@pbauman pbauman Use accessor API for FEMContext. c3ea18f
@pbauman pbauman Update solvers to use updated FEMContext API. 52b4023
@pbauman pbauman Need non-const versions of these accessors. I'm not convinced this
is the right way to go here, but rolling with it for now.
b72ce0d
@pbauman pbauman Updating reduced_basis to use new FEMContext API's. b2d82f4
@pbauman pbauman Assert on the private data structures. 72de92c
@pbauman pbauman Update to use new FEMContext API. Also needed a
non-const Elem&.
34266d6
@pbauman pbauman Return a reference to the QBase instead of a pointer. 9c757bd
@pbauman pbauman Now make everything protected/private. The library builds for me,
but the examples still need to be updated so likely make check
will fail at this point.
ac9dadd
@pbauman pbauman Updated adjoints_ex1 to use the new FEMContext API. a54c4b2
@pbauman pbauman Update adjoint_ex2 for new FEMContext API. e152bb7
@pbauman pbauman Update adjoints_ex3 to use new FEMContext API. b9c13e2
@pbauman pbauman Updated adjoint_ex4 to use updated FEMContext API. 4f1b73e
@pbauman pbauman Updated adjoints_ex5 to use updated FEMContext API. 1068ade
@pbauman pbauman Updated fem_system examples to use updated
FEMContext API.
8f02d31
@pbauman pbauman Updated reduced_basis examples to conform to new
FEMContext API.
0a6375c
@pbauman pbauman UPdated vector_fe examples to conform to new FEMContext API. 86336a2
@pbauman pbauman Patch from @dknez to fixed reduced_basis_ex2 for
FEMContext API updated.
6769426
@pbauman pbauman Patch from @dknez to fix reduced_basis_ex7 for
FEMContext API changes.
4318e29
@pbauman pbauman Revert "Now make everything protected/private. The library builds for…
… me,"

This reverts commit 248f1fa.
Will reapply after the 0.9.2 release.
3539428
@pbauman
libMesh - C++ Finite Element Library member

Rebased this guy after resolving a couple of conflict with the reduced_basis stuff. @benkirk, if this is the only thing holding up 0.9.2, then you'll be good after I merge this. Going to do another pull request to revert 3539428 that can be merged post 0.9.2.

@pbauman pbauman merged commit d25f5fc into libMesh:master Jul 2, 2013
@pbauman pbauman deleted the pbauman:privatize_fem_system branch Jul 2, 2013
@pbauman pbauman added a commit to pbauman/libmesh that referenced this pull request Jul 2, 2013
@pbauman pbauman Make objects in FEMContext private, forcing users
to the use API that was finalized in #108.
a60d303
@pbauman pbauman added a commit to pbauman/libmesh that referenced this pull request Jul 18, 2013
@pbauman pbauman Make objects in FEMContext private, forcing users
to the use API that was finalized in #108.
7400547
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment