-
Notifications
You must be signed in to change notification settings - Fork 67
Additions to DenseMatrixBase and MutableDenseMatrix #143
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
Conversation
9f7f7dc
to
cd46608
Compare
@isuruf Though I have updated the |
It's using the correct version of symengine. Something is wrong with your code here. |
symengine/lib/symengine.pxd
Outdated
@@ -671,6 +671,14 @@ cdef extern from "<symengine/matrix.h>" namespace "SymEngine": | |||
const DenseMatrix &x, DenseMatrix &result) nogil except + | |||
void diff "SymEngine::sdiff"(const DenseMatrix &A, | |||
RCP[const Basic] &x, DenseMatrix &result) nogil except + | |||
void row_join(const DenseMatrix &A, const DenseMatrix &B, DenseMatrix &C) nogil |
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 should be declared in the class itself without the first parameter. See add_matrix
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.
But add_matrix
by default has only two parameters. Changing the declarations similarly has no effect as well.
@isuruf There doesn't seem to be any effect with changing the declarations. What do you suggest we do? |
@isuruf Can you also take a look here. I'd like to finish this up before the release. |
This can't go in the release, since it's using functionality of symengine not yet released. I'll have a look about what can do. |
cbca8e1
to
22999fd
Compare
@isuruf Anything on this? I'd like to finish this up. |
…exchange_dense, dot and cross functions from SymEngine
Ping @isuruf. |
symengine/lib/symengine_wrapper.pyx
Outdated
@@ -2826,6 +2842,8 @@ cdef class DenseMatrixBase(MatrixBase): | |||
def tolist(self): | |||
return self[:] | |||
|
|||
_mat = tolist |
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 is really inefficient. _mat
method should just return self
.
symengine/lib/symengine.pxd
Outdated
void row_add_row_dense(DenseMatrix &A, unsigned i, unsigned j, RCP[const Basic] &c) nogil | ||
void column_exchange_dense(DenseMatrix &A, unsigned i, unsigned j) nogil | ||
void dot(const DenseMatrix &A, const DenseMatrix &B, DenseMatrix &C) nogil | ||
void cross(const DenseMatrix &A, const DenseMatrix &B, DenseMatrix &C) nogil |
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.
@isuruf Can you point out what is wrong with the declarations here?
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.
They are not declared in class DenseMatrix
. They should be declared outside the class.
Can you review this once @isuruf? |
You need to declare the function outside the class in symengine. |
@isuruf The tests pass now, can you review this please? |
Ping @isuruf. |
Relevant: #17