Skip to content
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

Get rid of ConvertToMatrixRep #246

Open
ssiccha opened this issue Mar 31, 2021 · 1 comment
Open

Get rid of ConvertToMatrixRep #246

ssiccha opened this issue Mar 31, 2021 · 1 comment

Comments

@ssiccha
Copy link
Collaborator

ssiccha commented Mar 31, 2021

Recog is using ConvertToMatrixRep all over the place. I think it would make more sense to call it on the input (maybe after copying it first) of RecogniseGroup and similar functions and avoid it after that.

@fingolfin
Copy link
Member

I am not sure why that should be a goal in itself. What problem does it solve?

Note that these operations are very fast if the matrix is already in compressed form, but accidentally working with a non-compressed matrix can seriously wreck performance. At the same time, all kinds of matrix manipulations can (accidentally) cause the matrix to revert to uncompressed form, or to being a list-of-compressed-vectors. So at the very least, one would want to replace those calls with assertions that verify the matrix is still compressed.

That said, of course ideally the MatrixObj project would progress and we'd change recog to work with arbitrary MatrixObj implementations, and to use all the relevant MatrixObj APIs (resp. to add such APIs as we need them); and then during the progress on this, these ConvertToMatrixRep would indeed become dedundant...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants