#17567: Cross product matrix (hat operator)
-------------------------------------+-------------------------------------
Reporter: gagern | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.5
Component: linear algebra | Resolution:
Keywords: | Merged in:
Authors: Martin von Gagern | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/gagern/ticket/17567 | 57cf33d49d93bb828261ab1a39a1ba3ff82cb256
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by gagern):
I'd like to leave my commit as is, for the following reasons.
Replying to [comment:5 tscrim]:
> A few comments. Please remove the `self` as input, it's not considered
as such.
I'm somehow not sure this is universally accepted.
{{{
$ git ls-files -z | xargs -0 grep -Ee '- +``self``[: ]' | wc -l
442
}}}
While I agree that one can gain little from a description like “`self` is
any object of the class we're talking about here”, the case at hand is
somewhat different, since there are extra requirements for that object.
Making them clear in the '''INPUT''' section seems important to me. If you
still disagree, I'd like this discussed on the dev list, unless you can
provide a convincing argument.
If this was already discussed on the list, I'd welcome some pointer.
[https://groups.google.com/d/msg/sage-devel/wCYdiMFyiH8/i3rZwywv9AEJ This
thread] gives an example with `self` in the docstring, and noone objected.
But the issue at hand was somethinbg different.
[https://groups.google.com/d/msg/sage-devel/087GdkT9n4g/Y9CIQwZBdiwJ This
post] suggests documenting “anything special about self”, but it might be
considered somewhat dated, or the opinion of a single person without any
discussion about it. It certainly contradicts the first thread I
referenced with regard to indentation.
So far I haven't found a discussion specifically about whether or not to
include `self` in the '''INPUT''' section, and no developer guide document
either.
> I would also consider using the
{{{:wikipedia:`Hat_operator#Cross_product`}}} in the documentation.
Considered it, and tried it out. But in the HTML text the hash (anchor
separator) looks ugly, and if I try to use it in the link target
annotation it doesn't work at all.
> A bit more substantial, I would instead check the rank by
`self.parent().rank()` and define `s` inside of each of the 2 cases.
As I said, I'm not optimizing for the case where the operation is not
defined, and in the other two cases we need the `s` array sooner or later,
so it might as well be sooner.
> for `ZZ`, which always copies the list, this could result in a big
speedup
The only way to avoid this list copy for the cases of rank 3 or 7 is by
doing element access on `self` instead of a (possibly copied) list of
entries. I did some comparison, my implementation against one (called
`cross_product_matrix2`) which replaces every occurrence of `s[i]` by
`self[i]`, does not set `s` at all, and uses `self.parent().rank()`
(cached to a variable) instead of `len(s)` for case distinction. The
difference is neglible, but slightly in favor of my code as it is here,
with the array copy.
{{{
sage: m = (ZZ^3).random_element()
sage: %timeit m.cross_product_matrix()
10000 loops, best of 3: 166 µs per loop
sage: %timeit m.cross_product_matrix2()
10000 loops, best of 3: 169 µs per loop
sage: m = (ZZ^7).random_element()
sage: %timeit m.cross_product_matrix2()
1000 loops, best of 3: 187 µs per loop
sage: %timeit m.cross_product_matrix()
10000 loops, best of 3: 182 µs per loop
}}}
> Plus this is more robust if the behavior is changed to mimic `ZZ` (i.e.,
it always copies the elements). I might also consider adding the
definition of `R` and `zero` inside of the 2 cases as well, but that is a
definite micro-optimization.
Same argument as above: we'll need them in either of the defined cases, so
let's share.
> Somewhat of a side note, constructing the basis might be an expensive
operation. So if you don't really need it, then let's stay away from using
it.
Glad you see it this way as well.
> Nice little patch; looks good otherwise.
Could I convince you? Will you give this a positive review even if I don't
change anything?
--
Ticket URL: <http://trac.sagemath.org/ticket/17567#comment:7>
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.
