search

MASTmultiphysics / mast-multiphysics

Multidisciplinary-design Adaptation and Sensitivity Toolkit (MAST) - Sensitivity-enabled multiphysics FEA for design
https://www.mast-multiphysics.com
GNU Lesser General Public License v2.1
43 stars 24 forks source link

Protected versus public residual methods #82

Open jdeaton opened 4 years ago

jdeaton commented 4 years ago

@manavbhatia One thing @JohnDN90 and I noticed was that there is a mix of protected and public residual/Jacobian methods in the elements. The protected ones (for example thermal_residual() in 1D structural elements) are called from within larger public residual methods. We found that this complicates testing these calculations in isolation (from the larger encompassing residual method) in our Catch2 test setup.

One current work-around we are using is adding #define protected public before including the relevant headers, but this seems to be a bit of a cludge and makes basically EVERYTHING public. Although I have found a lot of discussion in various C++ forums where it is common to use that along with #define private public.

In the interest of increasing testability, what do you think of making all residual calculation methods public in the elements?

JohnDN90 commented 4 years ago

I was contemplating this as I was writing the tests. I did some searching online at that time, and it seemed like the more popular school of thought was to only test your public interfaces. If something was broken in your private/protected methods, it should be caught through whichever public test would utilize those methods. Obviously, some users disagreed and recommended testing protected methods with #define protected public.

I decided to write the tests in a way that didn't just test the public interface, but in a way that would be useful for debugging as well, essentially a "developer" version of tests. For example, with the Jacobian (tangent stiffness) I wrote it so that we test each individual part, so that instead of getting "Jacobian is wrong" we can get "bending Jacobian is wrong" or "thermoelastic Jacobian is wrong", thus (hopefully) being able to locate bugs more easily.

Currently, I don't use #define protected public in too many places. It may not always be avoidable however. For instance, I use it in one test to check the shape function derivatives against a finite differenced result. In that particular case, I don't think it would make sense some of the protected methods I'm using public. So it might depend on the usage case. I don't have enough experience with #define protected public yet to know where or when it's going to cause issues.

As far as the residual calculation methods, it might make sense to make them public since that could be useful for other things. For example, if we ever do develop an OpenMDAO interface, it might be useful to have the partials of each discipline (dR/dU) available separately for that.

manavbhatia commented 4 years ago

I think it will be fine to move the residual methods from protected to public.

The API in place has accessed them through the methods to compute the volume and boundary integrals for external loads. This served the needs of the code sometime ago, but if the needs have evolved (testing, etc.) then the classification of the methods can be modified suitably.