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

[FEM.Elastic] Start unification of tetra FF and division of tests #4778

Merged
merged 16 commits into from
Jul 12, 2024

Conversation

alxbilger
Copy link
Contributor

@alxbilger alxbilger commented Jun 18, 2024

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 of if.

  • The 3 classes now inherit from a common base class BaseLinearElasticityFEMForceField. I moved everything that was common (only regarding the unit test. More common code is remaining) in this class. It includes:
  • A common 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 in FastTetrahedralCorotationalForceField and TetrahedralCorotationalFEMForceField 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

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).
@epernod
Copy link
Contributor

epernod commented Jun 18, 2024

ah... ok good.. it was in my todo list of next month :)

@sofabot
Copy link
Collaborator

sofabot commented Jun 19, 2024

[ci-depends-on] detected during build #2.

To unlock the merge button, you must

@alxbilger alxbilger added the pr: breaking Change possibly inducing a compilation error label Jun 19, 2024
@sofabot
Copy link
Collaborator

sofabot commented Jun 19, 2024

[ci-depends-on] detected during build #3.

To unlock the merge button, you must

@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests]

@sofabot
Copy link
Collaborator

sofabot commented Jun 19, 2024

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jun 21, 2024

[ci-depends-on] detected during build #5.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jun 21, 2024

[ci-depends-on] detected during build #6.

To unlock the merge button, you must

@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Jun 26, 2024
@fredroy fredroy added pr: status to review To notify reviewers to review this pull-request and removed pr: status ready Approved a pull-request, ready to be squashed labels Jul 2, 2024
@sofabot
Copy link
Collaborator

sofabot commented Jul 3, 2024

[ci-depends-on] detected during build #7.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jul 3, 2024

[ci-depends-on] detected during build #8.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jul 11, 2024

[ci-depends-on] detected during build #9.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jul 11, 2024

[ci-depends-on] detected during build #10.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jul 12, 2024

[ci-depends-on] detected during build #11.

To unlock the merge button, you must

@sofabot
Copy link
Collaborator

sofabot commented Jul 12, 2024

[ci-depends-on] detected during build #12.

To unlock the merge button, you must

@epernod
Copy link
Contributor

epernod commented Jul 12, 2024

[ci-build]

@sofabot
Copy link
Collaborator

sofabot commented Jul 12, 2024

[ci-depends-on] detected during build #13.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍

@epernod epernod added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Jul 12, 2024
@epernod epernod merged commit 9fda892 into sofa-framework:master Jul 12, 2024
9 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: breaking Change possibly inducing a compilation error pr: status ready Approved a pull-request, ready to be squashed refactoring Refactor code
5 participants