{"action":"created","issue":{"url":"https://api.github.com/repos/yixuan/spectra/issues/16","repository_url":"https://api.github.com/repos/yixuan/spectra","labels_url":"https://api.github.com/repos/yixuan/spectra/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/yixuan/spectra/issues/16/comments","events_url":"https://api.github.com/repos/yixuan/spectra/issues/16/events","html_url":"https://github.com/yixuan/spectra/issues/16","id":193776137,"number":16,"title":"Passing argument as `Ref<const _MatrixType>` instead of `const _MatrixType&`","user":{"login":"dariomangoni","id":8025529,"avatar_url":"https://avatars.githubusercontent.com/u/8025529?v=3","gravatar_id":"","url":"https://api.github.com/users/dariomangoni","html_url":"https://github.com/dariomangoni","followers_url":"https://api.github.com/users/dariomangoni/followers","following_url":"https://api.github.com/users/dariomangoni/following{/other_user}","gists_url":"https://api.github.com/users/dariomangoni/gists{/gist_id}","starred_url":"https://api.github.com/users/dariomangoni/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dariomangoni/subscriptions","organizations_url":"https://api.github.com/users/dariomangoni/orgs","repos_url":"https://api.github.com/users/dariomangoni/repos","events_url":"https://api.github.com/users/dariomangoni/events{/privacy}","received_events_url":"https://api.github.com/users/dariomangoni/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-12-06T13:46:48Z","updated_at":"2016-12-06T14:36:54Z","closed_at":null,"body":"I'm a first-time user of Eigen and Spectra, so what I'm experiencing may not be critical, even not correct, anyway...\r\n\r\nI need to solve a Generalized Eigenvalue Problem on CSR-format matrices.\r\nI _already_ have CSR arrays so I would like to wrap them in a `Eigen::SparseMatrix` in order to feed the `Spectra::SymGEigsSolver` by mean of `Spectra::SparseSymMatProd` and `Spectra::SparseCholesky`.\r\n\r\nTo wrap CSR C-style arrays I would use `Map<SparseMatrix<double>>(...CSRmatrixdata...)`,\r\nbut then I came to an empasse. How can I call the `SparseSymMatProd` constructor? There are few ways...\r\n\r\n1. pass the `Map<>` itself\r\n2. pass a `Ref<>` to the `Map<>`\r\n3. pass an `Eigen::SparseMatrix&` referring to `Map<>`\r\n4. pass an `Eigen::SparseMatrix&` referring to `Ref<>` that refers to the `Map<>`\r\n\r\nWhat happens?\r\nFact 1) inside the `SparseSymMatProd` constructor, a `const Eigen::SparseMatrix&` is initialized with the argument of the constructor.\r\ni.e. `const Eigen::SparseMatrix& m_mat = _argument_`\r\n\r\nFact 2a) these passages do _not_ call copy*\r\n`Map<>(CSRarrays)`\r\n`Ref<> = Map<>`\r\nand obviously `Eigen::SparseMatrix& = Eigen::SparseMatrix&`\r\n\r\nFact 2b) these passages _do_ call copy*\r\n`Eigen::SparseMatrix& = Ref<>`\r\n`Eigen::SparseMatrix& = Map<>`\r\n\r\n*in order to prove that, I compared the `valuePtr()` address with the original CSR value address\r\n\r\nNow:\r\nin [1] and [2]: the reference-initialization inside the constructor forces a copy, but suddenly the destructors of `SparseMatrix` first and `CompressedStorage` after are called and they **destroy the arrays of `m_mat`** that have just been created! The matrix is invalidated an no product can be actually performed.\r\n\r\nin [3] and [4]: everything works fine, the reference-initialization called _inside_ the constructor is of the last case of Fact 2a, so there is no copy and avoid this strange behaviour above, but there do is a copy in the reference initialization that is made before calling the constructor.\r\n\r\nSo:\r\nI would recommend to change the first lines of `Spectra::SparseSymMatProd`\r\nfrom\r\n```cplusplus\r\nprivate:\r\n  const SparseMatrix& m_mat;\r\npublic:\r\n    SparseSymMatProd(const SparseMatrix& mat_) :\r\n        m_mat(mat_) { }\r\n```\r\n\r\nto\r\n\r\n```cplusplus\r\nprivate:\r\n  Eigen::Ref<const SparseMatrix> m_mat;\r\npublic:\r\n    SparseSymMatProd(Eigen::Ref<const SparseMatrix> mat_) :\r\n        m_mat(mat_) { }\r\n```\r\n\r\nThis is retro-compatible: it works with `Eigen::SparseMatrix`\r\nand it should works also with `Ref`s\r\nand it doesn't force a copy.\r\n\r\nI'm not aware of all the implications and if this is actually correct.\r\nIf you, instead, have a better way to pass some arrays to the eigen solver without this mess and that doesn't force copies, please tell me! "},"comment":{"url":"https://api.github.com/repos/yixuan/spectra/issues/comments/265165116","html_url":"https://github.com/yixuan/spectra/issues/16#issuecomment-265165116","issue_url":"https://api.github.com/repos/yixuan/spectra/issues/16","id":265165116,"user":{"login":"yixuan","id":216398,"avatar_url":"https://avatars.githubusercontent.com/u/216398?v=3","gravatar_id":"","url":"https://api.github.com/users/yixuan","html_url":"https://github.com/yixuan","followers_url":"https://api.github.com/users/yixuan/followers","following_url":"https://api.github.com/users/yixuan/following{/other_user}","gists_url":"https://api.github.com/users/yixuan/gists{/gist_id}","starred_url":"https://api.github.com/users/yixuan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yixuan/subscriptions","organizations_url":"https://api.github.com/users/yixuan/orgs","repos_url":"https://api.github.com/users/yixuan/repos","events_url":"https://api.github.com/users/yixuan/events{/privacy}","received_events_url":"https://api.github.com/users/yixuan/received_events","type":"User","site_admin":false},"created_at":"2016-12-06T14:36:54Z","updated_at":"2016-12-06T14:36:54Z","body":"Hi @dariomangoni , thanks for the comments!\r\nThis is indeed a good point. `Map<SparseMatrix<double>>` and `Ref<SparseMatrix<double>>` are new features introduced in the recent Eigen 3.3, and they didn't exist before. I need to think of a way to make it compatible with older versions of Eigen, but at least for your own use, it's perfectly a good idea to make that change."}}
{"actor":{"display_login":"yixuan"}}