-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaPython] sparse matrix aliasing scipy/eigen #411
[SofaPython] sparse matrix aliasing scipy/eigen #411
Conversation
Hi Maxime, Thanks for the PR. I will have a look at it. |
Thank you @maxime-tournier and sorry for the delay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this PR breaking other's code and the use of the the contextmanager to write the python is elegant. There is an example file. The description is nice as the changelog.
Currently there is no tests but making one shouldn't be hard :)
@@ -139,6 +142,8 @@ struct DataTypeInfo | |||
{ | |||
} | |||
|
|||
// mtournier: wtf is this supposed to do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of comment are useless.
Drop them or maybe use a more formal approach so that we really get an answer of the use of this function or the deprecation the weird function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, those are my own personal comments and were not meant to make it in the final PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You often show your surprise with wtf comments which I found very funny because I have the same but I don't write them :)
@@ -251,6 +251,8 @@ if( SofaPython_FOUND ) | |||
mapping/PythonMultiMapping.cpp | |||
python/_Compliant.cpp | |||
python/Binding_AssembledSystem.cpp | |||
|
|||
# note: need to fix assembled solver in python before removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something that still to be done ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in this PR is loosely based on Compliant/python/python.cpp
, so I have to update the code there to use this PR's code to avoid duplication. Unfortunately little time to do so.
@hugtalbot I think this one should be merged. |
[ci-build] |
Also: the test is the associated example. |
Is the examples automatically started during CI tests ? |
Rebuild and run the scene tests : |
[ci-build] [with-scene-tests] |
This PR provides support for aliasing Eigen sparse matrices (classes derived from
EigenBaseSparseMatrix
) through scipy sparse matrices.The scipy matrix aliases the eigen matrix, so that any modification will reflect on both sides provided the sparsity pattern does not change (on either side). A python context manager is provided to commit modifications back, should the sparsity pattern change, as shown in the example below.
The binding is somewhat unconventional as it uses
ctypes
for simplicity. Perhaps a cleaner version could be made using regular bindings + passing Eigen matrix pointers through python capsules if someone is motivated.Example
Changelog
DataTypeInfo
forEigenBaseSparseMatrix
derived classesSofaPython/ctypes.cpp
for aliasing logicSofaPython.sparse
SofaPython/examples/sparse.py
This PR:
Reviewers will merge only if all these checks are true.