#17663: clean sparse matrices
-------------------------+-------------------------------------------------
Reporter: | Owner:
vdelecroix | Status: needs_review
Type: | Milestone: sage-6.5
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers:
linear algebra | Work issues:
Keywords: | Commit:
Authors: | aca0531f9ca26a3c32690032c4d7e170a4e5a5cb
Vincent Delecroix | Stopgaps:
Report Upstream: N/A |
Branch: |
public/17663 |
Dependencies: |
#17658 |
-------------------------+-------------------------------------------------
Comment (by gagern):
Interesting. Looks mostly good.
This is wrong, though:
{{{
for i from 0 <= i < self._nrows:
for j from 0 <= j < self._ncols:
if not x[k]:
entries[(i,j)] = x[k]
k += 1
}}}
It should be `if x[k]`: you want to copy the ''non''-zero elements. I had
some trouble testing this code path, though:
{{{
sage: R.<a,b>=QQ[]
sage: m1=matrix(2,2,[0,a,b,0],sparse=True); m1
[0 a]
[b 0]
sage: type(m1)(m1.parent(),[0,a,b,0],True,True)
[0 0]
[0 0]
}}}
I haven't yet figured out where the `m1` construction converts its
argument, but perhaps we can deprecate or even immediately remove that
code path dealing with a single flat list.
I also wonder whether we should have a custom cythonized type to be used
as key, instead of the generic CPython tuple or arbitrary size and types.
Might make things a lot faster and memory-efficient again.
As far as I can see, most other (non-generic) sparse matrix
implementations use something other than a dict to represent its entries.
Something like [http://en.wikipedia.org/wiki/Sparse_matrix#Yale Yale
format] as far as I can tell at a quick glance. Perhaps a really proper
cleanup should go all the way to using that? I'm not sure. Benefits are
better memory efficiency, but we'd get logarithmic lookup times as opposed
to the almost constant lookup in a hash table. And modifications would
become costly. So perhaps we should offer two alternatives, depending on
whether memory is an issue or not? Just thinking out loud, I'm not asking
for this to be implemented in the ticket at hand.
The whole class has poor docstring and doctest coverage. I know that when
adding new code, it should come with doctests these days. I'm not sure
whether your modifications are simply changing existing code and are
therefore exempt from this rule. I'd certainly be more happy if you were
to add docstrings with tests to all the methods you modified, taking care
to test all relevant code paths. To catch things like the one above.
--
Ticket URL: <http://trac.sagemath.org/ticket/17663#comment:2>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
