Mapped forcefield matrix + minor spelling changes #276

olivier-goury · 2017-05-19T16:14:15Z

To experiment a new handling of sparse matrices under mappings in our plugin, a few changes are needed in the core of SOFA.
Main changes:

Add a new function in MechanicalObject (commit 6a6fd98).
Refactoring of MatrixDerivId: warning holonomicC() is deprecated --> replaced by constraintMatrix (commit 966f9af).
Change allocation in createMatrix from a fullMatrix to a sparseMatrix.
Linked to Project SparseMatrices, Mapping & Mask : general discussion #26

This PR:

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

Reviewers will merge only if all these checks are true.

So that the subsetmultimapping function getJs() returns a compressed row sparse matrix (by default it returns an sparse EIGEN matrix, which is not compatible with csparse solvers, like sparseLDLSolver... )

…e deleting the matrix

buildIdentityBlocksInJacobian This function allows to put identity blocks in a MatrixDeriv of the mechanical state given a list of nodes + the ID of the MatrixDeriv

…MatrixAccessor that does not clear the mappedMatrices list so as not to initialise a new matrix at every timestep. This saves time when fine meshes are used. The new cheapClear function is used in MatrixLinearSolver. This commit may have side effects for some examples.

…ead of a compressed one.

Change the name of MatrixDerivId for using more meaningful names holonomicC() [now deprecated] => constraintMatrix() nonHolonomicC() [that was not used] => delete new type mappingMatrix() is introduced => use in some mapping to map the stiffness..

sofabot · 2017-05-19T16:14:18Z

Thank you for your pull request!
Someone will soon check it and start the builds.

…3d0e)

damienmarchal · 2017-05-19T16:29:12Z

Hi Olivier,

thank for your PR. Let's see of it build [ci-build].
I let other person to handle this PR as we are in the same team :)

damienmarchal · 2017-05-20T05:50:36Z

Hi @olivier-goury , @ChristianDuriez

Some tests on our CI are crashing with your PR, following the link
https://www.sofa-framework.org/dash/?branch=pr/mapped_forcefield_matrix
You will find the crashing tests, those are the one that have a negative runtime, in the stackstrace you can see it is related to some Eigen spare matrix operation.

matthieu-nesme

The visitor building sparse Jacobians is missing, and I still do not really get the point of these partially sparse Jacobians.
But despite the matrix vectors renaming, I do not see anything breaking the API, so it should not create troubles.

matthieu-nesme · 2017-05-22T07:19:22Z

SofaKernel/framework/sofa/core/VecId.h

@@ -150,8 +151,14 @@ class TStandardVec<V_MATDERIV, vaccess>
 public:
    typedef TVecId<V_MATDERIV, vaccess> MyVecId;

-    static MyVecId holonomicC()    { return MyVecId(1);}
-    static MyVecId nonHolonomicC() { return MyVecId(2);}
+    static MyVecId constraintMatrix()    { return MyVecId(1);} // jacobian matrix of constraints


The matrix names are really unclear. Would it be possible to have some doc about it?
'constraintMatrix' seems to be 'constraintJacobian'? Does that mean that only the mstates in the same node as a Constraint would have such a matrix?
Whereas 'mappingMatrix' should be 'sparseJacobian', and every mstates should got one?

Yes we can rename constraintMatrix by constraintJacobian it is perhaps more clear.
In this case, we could name the second mappingJacobian (to be coherent).

matthieu-nesme · 2017-05-22T07:26:14Z

SofaKernel/framework/sofa/core/behavior/BaseMechanicalState.h

@@ -226,6 +226,9 @@ class SOFA_CORE_API BaseMechanicalState : public virtual BaseState
    /// build the jacobian of the constraint in a baseMatrix
    virtual void getConstraintJacobian(const ExecParams* params, sofa::defaulttype::BaseMatrix* J,unsigned int & off) = 0;

+    /// fill the jacobian matrix (of the constraints) with identity blocks on the provided list of nodes(dofs)


For the usage I have in mind, it would be more useful to start building the sparse Jacobians from ForceFields/Constraints. I am not sure of the interest of starting from a full Identity.
Starting from a selection matrix (identity with 0s) corresponding to the dofs where the ForceFields/Constraints are contributing allows to have the as parse as possible Jacobians, possibly null for branches where there is no ForceFields/Constraints.
Note building such a selection matrix is actually managed by BaseForceField::updateForceMask.

Here the Identity is sparse and not necessary full (as you have a list of blocks).
It is implemented in the Mechanical State because these Jacobians are stored in the Mechanical State. But this function can be called, of course from ForceFields / Constraints

matthieu-nesme · 2017-05-22T07:27:45Z

SofaKernel/modules/SofaBaseMechanics/UniformMass.inl

@@ -75,13 +75,13 @@ using defaulttype::BaseMatrix;
 template <class DataTypes, class MassType>


This file is not really related to the PR, is it?

matthieu-nesme · 2017-05-22T07:27:55Z

SofaKernel/modules/SofaBaseLinearSolver/CompressedRowSparseMatrix.h

@@ -174,6 +174,11 @@ public :
    {
    }

+    ~CompressedRowSparseMatrix()


This file is not really related to the PR, is it?

you are right... but It is maybe useful to have a destructor for CompressedRowSparseMatrix that does a clear() before destruction.

matthieu-nesme · 2017-05-22T07:29:44Z

SofaKernel/modules/SofaBaseMechanics/MechanicalObject.inl

@@ -2623,6 +2623,32 @@ void MechanicalObject<DataTypes>::getConstraintJacobian(const core::ExecParams*
 }

 template <class DataTypes>
+void MechanicalObject<DataTypes>::buildIdentityBlocksInJacobian(const sofa::helper::vector<unsigned int>& list_n, core::MatrixDerivId &mID)


To me, it breaks the purpose of mstates to make them build matrices...
To me mstates should only store state vectors, but this idea seems definitively lost.

We can remove Jacobians from Mechanical States and store them in an other component.
But this would be an other modification of SOFA core.

matthieu-nesme · 2017-05-22T07:30:23Z

modules/SofaConstraint/LCPConstraintSolver.h

@@ -168,6 +168,8 @@ class SOFA_CONSTRAINT_API LCPConstraintSolver : public ConstraintSolverImpl
    Data<double> showCellWidth;
    Data<defaulttype::Vector3> showTranslation;
    Data<defaulttype::Vector3> showLevelTranslation;
+    Data< sofa::helper::vector<double> > delta;


are these new Data use somewhere?

Thank you for your comment...
My bad, again something that should not be included in the Pull Request

matthieu-nesme · 2017-05-22T07:33:25Z

modules/SofaMiscMapping/CMakeLists.txt

@@ -71,7 +71,12 @@ set(SOURCE_FILES

 add_library(${PROJECT_NAME} SHARED ${HEADER_FILES} ${SOURCE_FILES})
 target_link_libraries(${PROJECT_NAME} PUBLIC SofaSimpleFem SofaRigid)
+
+if(USE_COMPRESSED_ROW_SPARSE)


OK to have a new compilation variable 'USE_COMPRESSED_ROW_SPARSE'.
Indeed it is problematic, that we want the same code, with the same API, described with different matrix structure, and I have no elegant proposition to handle that.
But where would be defined USE_COMPRESSED_ROW_SPARSE, you defined it manually in your cmakecache?
To be clearer, would it be possible to simply add
add_definitions("-DUSE_COMPRESSED_ROW_SPARSE") rather than duplicating code.

My bad... It was an intermediate code that was useful to get getJs but we no longer use this function
This will be removed from the pull request

matthieu-nesme · 2017-05-22T07:33:45Z

modules/SofaMiscMapping/SubsetMultiMapping.inl

@@ -28,6 +28,7 @@
 #include <iostream>
 #include <SofaBaseMechanics/IdentityMapping.h>

+#define USE_COMPRESSED_ROW_SPARSE 1


DEFINITIVELY NOT in the code!

Same comment

…raintJacobian, and mappingMatrix for MappingJacobian. DELETE: Delete some useless datafields in LCPconstraintSolver.h

olivier-goury · 2017-05-29T11:59:01Z

Thank you, warnings removed!

olivier-goury · 2017-05-30T11:22:29Z

We took care of the comments, would you like to merge this PR or do you see other issues?
Thanks

hugtalbot · 2017-05-31T13:38:03Z

hey everyone,
we discussed it this morning at the dev meeting. But we were not competent for this. That's why we added @courtecuisse and @JeremieA to this PR. The idea is to understand if the PR is breaking or not the constraint API.

@courtecuisse @JeremieA could you help us guys ?
Sorry @olivier-goury and @ChristianDuriez for the delay.

hugtalbot · 2017-06-06T15:38:41Z

Without feedback of yours @courtecuisse and @JeremieA this will be merged tomorrow (7 days since we poked you)

matthieu-nesme · 2017-06-07T07:39:33Z

Please, before merging, I'd like to get the interest of this PR?

Like already said, it does not bring anything but break the API, and polluate BaseMechanicalState with a off-purpose virtual function. So better motivate these modifications before merging.

ChristianDuriez · 2017-06-07T08:03:45Z

Thank you for your comment
The interest is to be able to map the forcefields through the mapping in a sparse manner for many solvers of SOFA.

I agree that this functionality is not directly available in the API and for now, would be only available in a private plug-in. But this is work in progress and we need more time to have a clean code available.

Why does it break the API ?

…ction buildIdentityBlocksInJacobian in MechanicalObject

olivier-goury · 2017-06-09T16:05:42Z

We created a new tag SOFA_WITH_EXPERIMENTAL_FEATURES, set to OFF by default, which defines if the function buildIdentityBlocksInJacobian is used.
Hopefully, this PR can be merged now?

guparan · 2017-06-12T13:08:17Z

For some reason the latest commits have been ignored by the CI.
Let's [ci-build] them.

bcarrez · 2017-06-12T13:34:33Z

[ci-build]

damienmarchal · 2017-06-15T06:23:50Z

[ci-build]

ChristianDuriez and others added 16 commits December 2, 2016 15:37

Add a compilation tag: USE_COMPRESSED_ROW_SPARSE

362dd9c

So that the subsetmultimapping function getJs() returns a compressed row sparse matrix (by default it returns an sparse EIGEN matrix, which is not compatible with csparse solvers, like sparseLDLSolver... )

add a destructor of CompressedRowSparseMatrix that call clear() befor…

5967064

…e deleting the matrix

add a new function in Mechanical State:

6a6fd98

buildIdentityBlocksInJacobian This function allows to put identity blocks in a MatrixDeriv of the mechanical state given a list of nodes + the ID of the MatrixDeriv

Correct bug in buildIdentityBlocksInJacobian

c5d6a13

Merge branch 'master' into mapped_forcefield_matrix

cffc7b7

Minor optimization in buildIdentityBlocksInJacobian

6a899f7

Correct bug in create matrix which used to allocate a fullmatrix inst…

880522f

…ead of a compressed one.

Merge branch 'master' into mapped_forcefield_matrix

d465b22

Merge branch 'master' into mapped_forcefield_matrix

6ffd8a6

Minor Changes

d41bc34

Merge branch 'master' into mapped_forcefield_matrix

93bb530

Merge branch 'master' into mapped_forcefield_matrix

d7ccfa4

FIX: put a realistic value for the friction

7703d0e

REFACTORING:

966f9af

Change the name of MatrixDerivId for using more meaningful names holonomicC() [now deprecated] => constraintMatrix() nonHolonomicC() [that was not used] => delete new type mappingMatrix() is introduced => use in some mapping to map the stiffness..

update deprecated message for holonomicC()

a5d7253

FIX: put a realistic value for the friction (reverted from commit 770…

4d85dfd

…3d0e)

damienmarchal requested a review from matthieu-nesme May 19, 2017 16:29

damienmarchal assigned guparan May 19, 2017

damienmarchal added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request project : Mapping & Mask labels May 20, 2017

matthieu-nesme reviewed May 22, 2017

View reviewed changes

olivier-goury added 3 commits May 23, 2017 18:28

FIX: Remove compilation tag USE_COMPRESSED_ROW_SPARSE

afc862c

FIX: Remove Compilation Tag USE_COMPRESSED_ROW_SPARSE from CmakeList

b887543

REFACTORING:Change the name MatrixDerivId::constraintMatrix for const…

3c6c686

…raintJacobian, and mappingMatrix for MappingJacobian. DELETE: Delete some useless datafields in LCPconstraintSolver.h

olivier-goury changed the title ~~Mapped forcefield matrix~~ Mapped forcefield matrix + minor spelling changes May 24, 2017

FIX: Remove unnecessary warnings

36f367c

hugtalbot assigned courtecuisse and JeremieA and unassigned guparan May 31, 2017

damienmarchal requested review from courtecuisse and JeremieA June 1, 2017 20:33

damienmarchal assigned hugtalbot and unassigned courtecuisse and JeremieA Jun 1, 2017

hugtalbot mentioned this pull request Jun 7, 2017

Project SparseMatrices, Mapping & Mask : general discussion #26

Open

olivier-goury added 3 commits June 9, 2017 17:37

Add tag SOFA_WITH_EXPERIMENTAL_FEATURES related to the use of the fun…

7084307

…ction buildIdentityBlocksInJacobian in MechanicalObject

Merge branch 'master' into mapped_forcefield_matrix

5e98b92

Small fix on the new compilation tag SOFA_WITH_EXPERIMENTAL_FEATURES

8cb787d

FIX: small bug with the compilation tag SOFA_WITH_EXPERIMENTAL_FEATURES

27ca0cf

guparan 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 14, 2017

bcarrez merged commit a096eb8 into sofa-framework:master Jun 16, 2017

guparan added this to the v17.06 milestone Jun 29, 2017

damienmarchal deleted the mapped_forcefield_matrix branch July 5, 2017 21:21

olivier-goury mentioned this pull request Jul 27, 2018

[all] Sofa add mechanical matrix mapper #721

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mapped forcefield matrix + minor spelling changes #276

Mapped forcefield matrix + minor spelling changes #276

olivier-goury commented May 19, 2017 •

edited by guparan

Loading

sofabot commented May 19, 2017

damienmarchal commented May 19, 2017

damienmarchal commented May 20, 2017

matthieu-nesme left a comment

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

matthieu-nesme May 22, 2017

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

matthieu-nesme May 22, 2017

ChristianDuriez May 23, 2017

olivier-goury commented May 29, 2017

olivier-goury commented May 30, 2017

hugtalbot commented May 31, 2017

hugtalbot commented Jun 6, 2017

matthieu-nesme commented Jun 7, 2017

ChristianDuriez commented Jun 7, 2017

olivier-goury commented Jun 9, 2017

guparan commented Jun 12, 2017

bcarrez commented Jun 12, 2017

damienmarchal commented Jun 15, 2017

		@@ -75,13 +75,13 @@ using defaulttype::BaseMatrix;
		template <class DataTypes, class MassType>

Mapped forcefield matrix + minor spelling changes #276

Mapped forcefield matrix + minor spelling changes #276

Conversation

olivier-goury commented May 19, 2017 • edited by guparan Loading

sofabot commented May 19, 2017

damienmarchal commented May 19, 2017

damienmarchal commented May 20, 2017

matthieu-nesme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivier-goury commented May 29, 2017

olivier-goury commented May 30, 2017

hugtalbot commented May 31, 2017

hugtalbot commented Jun 6, 2017

matthieu-nesme commented Jun 7, 2017

ChristianDuriez commented Jun 7, 2017

olivier-goury commented Jun 9, 2017

guparan commented Jun 12, 2017

bcarrez commented Jun 12, 2017

damienmarchal commented Jun 15, 2017

olivier-goury commented May 19, 2017 •

edited by guparan

Loading