Opened 6 years ago
Closed 6 years ago
#17583 closed enhancement (fixed)
Clean up free module elements
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  dcbf770 (Commits)  Commit:  dcbf770938972358639fc637dc0bf99ddf2489d6 
Dependencies:  #17561, #10779  Stopgaps: 
Description (last modified by )
General cleanup using modern Cython code in preparation for followup tickets.
No changes in functionality (except perhaps details).
Change History (16)
comment:1 Changed 6 years ago by
 Description modified (diff)
comment:2 Changed 6 years ago by
 Branch set to u/jdemeyer/ticket/17583
 Created changed from 01/05/15 13:26:20 to 01/05/15 13:26:20
 Modified changed from 01/05/15 14:31:23 to 01/05/15 14:31:23
comment:3 Changed 6 years ago by
 Commit set to 66d2f76d685f654f18b77f0051ae85235a4f846f
comment:4 Changed 6 years ago by
 Dependencies changed from #10513, #17561 to #17561
 Status changed from new to needs_review
comment:5 followup: ↓ 6 Changed 6 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to needs_info
_sparse_dot_product()
(which you didn't touch) appears not to be used anywhere and could probably be deleted.
By the way, is there a reason to specialcase the vectors of length zero in FreeModuleElement.dot_product()
?
Otherwise lgtm.
comment:6 in reply to: ↑ 5 ; followup: ↓ 9 Changed 6 years ago by
Replying to mmezzarobba:
By the way, is there a reason to specialcase the vectors of length zero in
FreeModuleElement.dot_product()
?
Yes, to apply the following optimization: instead of starting with zero and a[i]*b[i]
, we start with a[0] * b[0]
and add a[i]*b[i]
for i >= 1
.
I'm actually preparing a commit which refactors dot products, since the logic was a bit strange. With the new commit (still testing), there will be a cpdef
method _dot_product_
for equal parents and _dot_product_coerce_
for parents which might be different.
comment:7 Changed 6 years ago by
 Commit changed from 66d2f76d685f654f18b77f0051ae85235a4f846f to a6d357a8b2874aaec970aa0d2afc3fa14d60969e
Branch pushed to git repo; I updated commit sha1. New commits:
4c99ae2  Improve dot products

c84246e  #10779: Improve coverage test for structure/element.pyx

db023d4  Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779

2e30025  trac #10779 corrected 3 doctests

9d72c81  Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779

9531937  trac #10779 correct failing doctest + more doc changes

54396cd  Merge branch 'u/chapoton/10779' into 6.4

dbf722e  Remove gcd, lcm, xgcd from element.pyx

a6d357a  Merge ticket/10779 into ticket/17583

comment:8 Changed 6 years ago by
 Status changed from needs_info to needs_review
comment:9 in reply to: ↑ 6 ; followups: ↓ 10 ↓ 11 Changed 6 years ago by
Replying to jdemeyer:
Yes, to apply the following optimization: instead of starting with zero and
a[i]*b[i]
, we start witha[0] * b[0]
and adda[i]*b[i]
fori >= 1
.
Fine. (I was not sure that it would be faster...)
Regarding your last commit, I agree with the changes, but I don't understand why dot_product
is defined in FreeModuleElement
rather than in Vector
. Actually I don't understand the point of the distinction between Vector
and FreeModuleElement
at all.
Also, I think we can do two additional simplifications related to dot_product
:
diff git a/src/sage/matrix/matrix_generic_sparse.pyx b/src/sage/matrix/matrix_generic_sparse.pyx index b2ff48a..a4cf07c 100644  a/src/sage/matrix/matrix_generic_sparse.pyx +++ b/src/sage/matrix/matrix_generic_sparse.pyx @@ 495,17 +495,6 @@ def Matrix_sparse_from_rows(X): return M(entries, coerce=False, copy=False) ## def _sparse_dot_product(v, w): ## """ ## INPUT: ## v and w are dictionaries with integer keys. ## """ ## x = set(v.keys()).intersection(set(w.keys())) ## a = 0 ## for k in x: ## a = a + v[k]*w[k] ## return a  cdef _convert_sparse_entries_to_dict(entries): e = {} for i in xrange(len(entries)): diff git a/src/sage/modules/vector_double_dense.pyx b/src/sage/modules/vector_double_dense.pyx index 9efe536..279ce58 100644  a/src/sage/modules/vector_double_dense.pyx +++ b/src/sage/modules/vector_double_dense.pyx @@ 389,8 +389,6 @@ cdef class Vector_double_dense(free_module_element.FreeModuleElement): sage: w*v 1.0 """  if not right.parent() == self.parent():  right = self.parent().ambient_module()(right) if self._degree == 0: from copy import copy return copy(self)
comment:10 in reply to: ↑ 9 ; followup: ↓ 15 Changed 6 years ago by
Replying to mmezzarobba:
Replying to jdemeyer:
Yes, to apply the following optimization: instead of starting with zero and
a[i]*b[i]
, we start witha[0] * b[0]
and adda[i]*b[i]
fori >= 1
.Fine. (I was not sure that it would be faster...)
It's always going to be somewhat faster and it avoids extra work in case the parents are not equal.
Regarding your last commit, I agree with the changes, but I don't understand why
dot_product
is defined inFreeModuleElement
rather than inVector
.
I could easily move it...
Actually I don't understand the point of the distinction between
Vector
andFreeModuleElement
at all.
Currently in Sage, the only Vector
s are also FreeModuleElement
s. But one could envision a parent which is not a free module...
Also, I think we can do two additional simplifications related to
dot_product
:diff git a/src/sage/matrix/matrix_generic_sparse.pyx b/src/sage/matrix/matrix_generic_sparse.pyx index b2ff48a..a4cf07c 100644  a/src/sage/matrix/matrix_generic_sparse.pyx +++ b/src/sage/matrix/matrix_generic_sparse.pyx @@ 495,17 +495,6 @@ def Matrix_sparse_from_rows(X): return M(entries, coerce=False, copy=False) ## def _sparse_dot_product(v, w): ## """ ## INPUT: ## v and w are dictionaries with integer keys. ## """ ## x = set(v.keys()).intersection(set(w.keys())) ## a = 0 ## for k in x: ## a = a + v[k]*w[k] ## return a  cdef _convert_sparse_entries_to_dict(entries): e = {} for i in xrange(len(entries)):
Absolutely not on this ticket, but see #17663.
comment:11 in reply to: ↑ 9 Changed 6 years ago by
Replying to mmezzarobba:
Regarding your last commit, I agree with the changes, but I don't understand why
dot_product
is defined inFreeModuleElement
rather than inVector
.
For consistency, I would prefer to keep dot_product
in FreeModuleElement
. For pairwise_product()
(which I didn't change), we also have pairwise_product
on the level of FreeModuleElement
but _pairwise_product_
on the level of Vector
. Further refactoring would need to involve all kinds of products (dot, cross and pairwise) and I would prefer not to do this now.
comment:12 Changed 6 years ago by
 Commit changed from a6d357a8b2874aaec970aa0d2afc3fa14d60969e to dcbf770938972358639fc637dc0bf99ddf2489d6
Branch pushed to git repo; I updated commit sha1. New commits:
dcbf770  Fix zerodimensional dot product

comment:13 Changed 6 years ago by
 Dependencies changed from #17561 to #17561, #10779
comment:14 Changed 6 years ago by
Despite the fact that "Trac's automerging failed", the branch actually merges when using git
.
comment:15 in reply to: ↑ 10 Changed 6 years ago by
 Status changed from needs_review to positive_review
Replying to jdemeyer:
Currently in Sage, the only
Vector
s are alsoFreeModuleElement
s. But one could envision a parent which is not a free module...
Sure, but either its elements would derive direclty from ModuleElement
, or (I'd say) much of what FreeModuleElement
currently does would need to be moved to Vector
. Anyway, if you feel that dot_product
should stay in FreeModuleElement
until someone does this kind of refactoring, I'm fine with that.
Thanks for you work on this ticket!
comment:16 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/17583 to dcbf770938972358639fc637dc0bf99ddf2489d6
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Merge commit '106f5d41398794b13a6a683236cf219d49602312' into HEAD
Clean up free module elements