overflow for vector mod2 dense

[videlec]

sage: VS = VectorSpace(GF(2),1)
sage: VS([2**64])
Traceback (most recent call last):
...
OverflowError: Python int too large to convert to C long

Component: linear algebra

Author: Vincent Delecroix

Branch/Commit: 428c88e

Reviewer: Jeroen Demeyer, Travis Scrimshaw

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

[videlec]

Author: Vincent Delecroix

[videlec]

Branch: u/vdelecroix/21746

[sagetrac-git]

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

9a52272

21746: fix overflow for dense vector mod 2

[sagetrac-git]

Commit: 9a52272

[jdemeyer]

Reviewer: Jeroen Demeyer

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

d1c54b1

21746: fix doctest in coding/

[sagetrac-git]

Changed commit from 9a52272 to d1c54b1

[videlec]

comment:5

Fixed doctest failure in coding/. Needs review again.

[tscrim]

comment:6

This branch does result in ~10% slowdown:

sage: V = VectorSpace(GF(2), 2^10)
sage: %timeit V([1]*(2^10))
The slowest run took 5.95 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 49.8 µs per loop

Before:

sage: V = VectorSpace(GF(2), 2^10)
sage: %timeit V([1]*(2^10))
The slowest run took 5.69 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 45.8 µs per loop

However, changing the for loop over enumerate back to for i from 0 <= i < self._degree: and setting xi = x[i] doesn't have a noticeable impact. So I think we have to deal with this.

I do have one nitpick: change the ZeroDivisionError to start with a lowercase. Once that is done, you can put this back to a positive review.

[sagetrac-git]

Changed commit from d1c54b1 to 428c2cd

[sagetrac-git]

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

428c2cd

21746: remove enumerate + cleaning

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

65894e5

21746: remove enumerate + cleaning

[sagetrac-git]

Changed commit from 428c2cd to 65894e5

[videlec]

comment:9

Thanks for the timings!

I removed the enumerate and also did some cleaning in the case x = 0. This code is completely useless

for i from 0 <= i < self._degree:
    mzd_set_ui(self._entries, 0)

[jdemeyer]

comment:10

The enumerate is optimized by Cython, so no need to remove it!

[tscrim]

comment:11

I realize the phrasing in my comment was somewhat vague too; I meant we have to just live with the slowdown. I did come to the conclusion that enumerate was likely optimized by Cython, but it is good to know. Anyways, removing it is a net-zero change, so I'm setting this back to a positive review.

[tscrim]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw

[vbraun]

comment:12

segfault galore... see patchbot

[videlec]

comment:13

Indeed. I am able to reproduce it with the shorter

sage: (GF(2)**0).zero_vector()

[sagetrac-git]

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

428c88e

21746: fix segementation fault for 0-dim spaces

[sagetrac-git]

Changed commit from 65894e5 to 428c88e

[vbraun]

Changed branch from u/vdelecroix/21746 to 428c88e