#17567: Cross product matrix (hat operator)
-------------------------+-------------------------------------------------
Reporter: | Owner:
gagern | Status: needs_review
Type: | Milestone: sage-6.5
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers: Travis Scrimshaw
linear algebra | Work issues:
Keywords: | Commit:
Authors: | 6db8fb0c0e1ed4eb334d059a749d3c45c7adcb94
Martin von Gagern | Stopgaps:
Report Upstream: N/A |
Branch: |
u/tscrim/17567 |
Dependencies: |
-------------------------+-------------------------------------------------
Changes (by tscrim):
* commit: 57cf33d49d93bb828261ab1a39a1ba3ff82cb256 =>
6db8fb0c0e1ed4eb334d059a749d3c45c7adcb94
* branch: u/gagern/ticket/17567 => u/tscrim/17567
* reviewer: => Travis Scrimshaw
Comment:
Sorry, this had fallen off my todo list.
Replying to [comment:7 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.
Most (all?) of that is in legacy code, and unfortunately, I recall this
discussion happening on a trac ticket and I don't remember which one. To
me, the requirements are clear both from the preceding paragraph and the
error message. Moreover, `self` is always referring to the class in
question and is not given as part of the input of the ''method'' (it is of
the ''function'' via the indirection, but that's a technical detail that a
user isn't likely to know or care about IMO). Also it's a more general
python thing: http://sphinxcontrib-
napoleon.readthedocs.org/en/latest/example_google.html. If you think this
still warrants a discussion on sage-devel, feel free to start one.
In the same vein, I'm not sure an `ArithmeticError` is the proper type of
error as no arithmetic is being performed in contrast with
`cross_product`. I'm thinking this should actually be a `ValueError` since
the given object does not
> > 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.
The integrated link does look better.
> > 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.
It's slightly interesting to me that the copy of the list is slower than
the overhead of the `__getitem__`. However for sparse vectors, I noticed a
clear slowdown by not using the list.
> Could I convince you? Will you give this a positive review even if I
don't change anything?
I ended up getting ~10% speedup by using a direct call to `MatrixSpace`:
{{{
sage: m = (ZZ^7).random_element()
sage: %timeit m.cross_product_matrix()
1000 loops, best of 3: 507 µs per loop
}}}
Before:
{{{
sage: %timeit m.cross_product_matrix()
1000 loops, best of 3: 552 µs per loop
}}}
I also used the `self.parent().rank()` and put the creation of both the
matrix space and getting the list inside each of the 2 statements because
creating a parent can be a (relatively) expensive operation and same for
the list. It's an extra 2 lines of (simple) code that could result in a
major speedup in case someone was trying to call this method on a long
vector and was catching the thrown error.
----
New commits:
||[http://git.sagemath.org/sage.git/commit/?id=6db8fb0c0e1ed4eb334d059a749d3c45c7adcb94
6db8fb0]||{{{Some tweaks and speedups.}}}||
--
Ticket URL: <http://trac.sagemath.org/ticket/17567#comment:8>
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.
