Replace =0 by is_zero() in test for matrix being alternating

[darijgr]

This branch does a couple small improvements to src/sage/matrix/matrix0.pyx, the most important one being a replacement of "== 0" by "== zero" since there are rings R whose zero element is not the same as R(0). (Two grammar errors in the doc are also fixed.)

CC: @nbruin @stumpc5

Component: linear algebra

Author: Darij Grinberg

Branch/Commit: f36f0ad

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/16306

[videlec]

comment:3

Hi Darij,

1. Could you provide examples of rings for which the test fails? It is not clear to me why we need this and if you do not want that such bug reproduces you need to test it. More precisely, in the documentation write something along the lines of

In the following example, we show that we solve the issue in :trac:`16306` is solved::

    sage: a_nice_example()
    Hey!

2. why not

-            if self.get_unsafe(i,i) != 0:
+            if self.get_unsafe(i,i):

Do we know that __nonzero__ always does what we expect?

Vincent

[darijgr]

comment:4

1. I don't have good examples. We have semirings where 0 is not the zero in Sage (try TropicalSemiring(ZZ)), but this particular method doesn't make much sense for these. This is more an issue of principle: We might eventually get rings like Grothendieck's lambda-rings (the ring of power series over a given ring K, with multiplication of power series as addition, and a weirder multiplication -- and, before you ask, there are finite-dimensional quotients of this, so it's not all lazy power series, so testing for zeroness does make sense), on which there will suddenly be a lot of failing code due to the fact that the coerce of 1 (not the coerce of 0) is the zero element. Better to fix as many of these issues as we can right now.

However, in most examples I've tried, is_zero is faster than == 0 due to the coercion that has to be resolved for the latter.

2. I don't know. I wouldn't make an assumption on the boolean coerce of a ring element. But maybe I could...

[videlec]

comment:5

Replying to @darijgr:

1. I don't have good examples. We have semirings where 0 is not the zero in Sage (try TropicalSemiring(ZZ)), but this particular method doesn't make much sense for these. This is more an issue of principle: We might eventually get rings like Grothendieck's lambda-rings (the ring of power series over a given ring K, with multiplication of power series as addition, and a weirder multiplication -- and, before you ask, there are finite-dimensional quotients of this, so it's not all lazy power series, so testing for zeroness does make sense), on which there will suddenly be a lot of failing code due to the fact that the coerce of 1 (not the coerce of 0) is the zero element. Better to fix as many of these issues as we can right now.

So, I do not understand the description of the ticket. You wrote Not all rings R have their zero element equal to R(0). But now that I asked for an example, you say that no such ring exists.

However, in most examples I've tried, is_zero is faster than == 0 due to the coercion that has to be resolved for the latter.

Ok. Looks like a better argument than claiming that there are other rings. Could you provide timings then?

2. I don't know. I wouldn't make an assumption on the boolean coerce of a ring element. But maybe I could...

No. I was just asking. Because, if you care about speed, this is by far the fastest.

Vincent

[darijgr]

comment:6

I hope you will agree that "all rings" and "all rings in Sage" are two rather different things... :)

Some timings on zero elements:

sage: %timeit ZZ(1) == 0
1000000 loops, best of 3: 419 ns per loop
sage: %timeit ZZ(1).is_zero()
1000000 loops, best of 3: 389 ns per loop
sage: %timeit QQ(1) == 0
100000 loops, best of 3: 2.15 µs per loop
sage: %timeit QQ(1).is_zero()
1000000 loops, best of 3: 853 ns per loop
sage: P.<x> = PolynomialRing(QQ)
sage: %timeit x == 0
100000 loops, best of 3: 2.76 µs per loop
sage: %timeit x.is_zero()
1000000 loops, best of 3: 223 ns per loop
sage: Sym = SymmetricFunctions(QQ); e = Sym.e(); ez = e.zero()
sage: %timeit ez == 0
10000 loops, best of 3: 27.9 µs per loop
sage: %timeit ez.is_zero()
100000 loops, best of 3: 4.45 µs per loop

and on nonzero elements:

sage: %timeit ZZ(2) == 0     
1000000 loops, best of 3: 416 ns per loop
sage: %timeit ZZ(2).is_zero()
1000000 loops, best of 3: 383 ns per loop
sage: %timeit QQ(2) == 0
100000 loops, best of 3: 2.15 µs per loop
sage: %timeit QQ(2).is_zero()
1000000 loops, best of 3: 877 ns per loop
sage: e2 = e[2]
sage: %timeit e2 == 0
10000 loops, best of 3: 27.8 µs per loop
sage: %timeit e2.is_zero()
100000 loops, best of 3: 5.91 µs per loop
sage: %timeit Integer(2) == 0     
1000000 loops, best of 3: 417 ns per loop
sage: %timeit Integer(2).is_zero()
1000000 loops, best of 3: 360 ns per loop

I don't know how much of the coercion overhead can be removed by improving the coercion system, but for now it looks like is_zero wins against coercion.

[darijgr]

comment:7

Actually, checking equality with parent.zero() is even faster than checking is_zero(). I'll push this once I am back home.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

0cf0ac9	Merge branch 'public/combinat/matrix0-fix' of git://trac.sagemath.org/sage into matrixnew
46f627e	speed up is_alternating a bit more, and improving some doc

[sagetrac-git]

Changed commit from 4d4cbe0 to 46f627e

[darijgr]

comment:10

Fixed. Sorry for it taking such a while!

[videlec]

comment:11

Replying to @darijgr:

Actually, checking equality with parent.zero() is even faster than checking is_zero(). I'll push this once I am back home.

I see, the following tests are more appropriate

sage: n = 12
sage: %timeit n.is_zero()
1000000 loops, best of 3: 204 ns per loop
sage: zero = n.parent().zero()
sage: %timeit n == zero
10000000 loops, best of 3: 149 ns per loop

[videlec]

comment:12

Hi Darij,

What is the point of if not self.get_unsafe(i,i) == zero: instead of if self.get_unsafe(i,i) != zero:? Could you write an appropriate title and an appropriate description for the ticket?

I agree that it is a small ticket. But it is not a reason to not do it cleanly!

Thanks,
Vincent

[sagetrac-git]

Changed commit from 46f627e to f36f0ad

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f36f0ad

use != instead of not ==

[darijgr]

comment:14

Oops, the use of "not" was a relic of the old version.

Part of the reason why comparison with parent().zero() takes so long is that parent() and zero() have to be computed. This can be done once per call of the method, and so the time amortizes quite well.

---

New commits:

f36f0ad

use != instead of not ==

[videlec]

comment:15

Hi Darij,

I still do not understand the motivation for the ticket: the function is not time critical and as you confirmed there is nowhere in Sage a ring whose zero is not 0. The code is better with the branch applied so I set to positive review.

Vincent

[darijgr]

comment:16

Thank you!

This is a really minor change I wouldn't have done if the code in question wasn't my code from an older ticket.

[vbraun]

comment:17

reviewer name

[darijgr]

Reviewer: ​Vincent Delecroix

[tscrim]

comment:19

FTR, there is one:

sage: T = TropicalSemiring(QQ)
sage: T(0)
0
sage: T.zero()
+infinity

(which we have to do because TestSuite uses zero as the additive identity, so we might want a separate categories for tropical stuff, but that's an issue for a different ticket).

[vbraun]

Changed reviewer from ​Vincent Delecroix to Vincent Delecroix

[vbraun]

Changed branch from public/combinat/matrix0-fix to f36f0ad