-
Notifications
You must be signed in to change notification settings - Fork 317
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
[FEM.Elastic] Start unification of tetra FF and division of tests #4778
Conversation
ah... ok good.. it was in my todo list of next month :) |
[ci-depends-on] detected during build #2. To unlock the merge button, you must
|
[ci-depends-on] detected during build #3. To unlock the merge button, you must
|
[ci-build][with-all-tests] |
[ci-depends-on] detected during build #4. To unlock the merge button, you must
|
[ci-depends-on] detected during build #5. To unlock the merge button, you must
|
[ci-depends-on] detected during build #6. To unlock the merge button, you must
|
[ci-depends-on] detected during build #7. To unlock the merge button, you must
|
[ci-depends-on] detected during build #8. To unlock the merge button, you must
|
[ci-depends-on] detected during build #9. To unlock the merge button, you must
|
[ci-depends-on] detected during build #10. To unlock the merge button, you must
|
[ci-depends-on] detected during build #11. To unlock the merge button, you must
|
[ci-depends-on] detected during build #12. To unlock the merge button, you must
|
[ci-build] |
[ci-depends-on] detected during build #13. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
3 different components for linear elasticity on tetrahedra:
FastTetrahedralCorotationalForceField
TetrahedralCorotationalFEMForceField
TetrahedronFEMForceField
The 3 components have similarities (even duplicated code), but don't share anything. It's time for refactoring!
My focus was the refactoring of the unit tests. Before the PR, a unique class tested the 3 components using a series of
if
. I refactored the 3 classes in order to reduce the specific processing in the unit test, i.e. reduce the number ofif
.BaseLinearElasticityFEMForceField
. I moved everything that was common (only regarding the unit test. More common code is remaining) in this class. It includes:Data
for Young's modulus and Poisson's ratio. This way, they share type, the same default value, and the same description. Note that Young's modulus inFastTetrahedralCorotationalForceField
andTetrahedralCorotationalFEMForceField
was a scalar, and now it's a vector of scalars. This requires to add a common way to access the Young's modulus of the i-th element. This is also a breaking change.This allows to define a typed unit test for all 3 classes. The particularities of the classes are defined in a derived class of the unit test. To me, a great advantage, is that we identify clearly that there are unit tests for the 3 components, because there a file for each class. It was not obvious before.
[ci-depends-on https://github.com/SofaDefrost/ModelOrderReduction/pull/133]
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if