[WIP] Support Eigen::Map<Eigen::SparseMatrix> #782
Conversation
|
Hi Alec, thank you very much for working on this, zero-copy sparse matrix passing will be useful. There were still some internal comments and questions inside the PR, so I am assuming that it is still in progress. Please let me know if/when you'd like me to do a review. |
| object matrix_type = module_::import_("scipy.sparse").attr(RowMajor ? "csr_matrix" : "csc_matrix"); | ||
| if (!obj.type().is(matrix_type)) | ||
| obj = matrix_type(obj); | ||
| } catch (const python_error &) { |
There was a problem hiding this comment.
This is a comment on lines 178-180. I might not be understanding this precisely, but doesn't this bit of code attempt to convert (line 180) obj to a sparse matrix if it is not already one? If so, this might cause issues where an overloaded routine that could take either a dense or sparse matrix, if the sparse matrix routine is considered first, anything that can be converted to a sparse matrix via csr_matrix (such as an int, float, or dense matrix) would then be converted to a sparse matrix and that's probaby not desirable. A simple fix would be to just fail (return false) if obj.type().is(matrix_type) is not true.
There was a problem hiding this comment.
Sorry, another minor comment on the above. If it is the case that you do end up returning false if !obj.type().is(matrix_type) then the catch block of the exception I'd guess should probably re-throw the exception since it probably means something is wrong other than that the object is not a sparse matrix (e.g., an import or some other error). I.e., the return of true/false should probably (unless I don't understand things) only happen if there are no errors. Similarly, in the below, when you are returning false, this means that it passes the above type check but there is still a problem getting, say, indices, and I'd guess that would mean something is quite wrong (not well indicated by a false return) with the object and/or setup.
If completed would fix #781.
Phew, I learned a lot poking around, but I'm mostly just cribbing the original Eigen::SparseMatrix support. I have barely tested any of this. I will use this PR to add tests etc.