Opened 10 years ago
Closed 10 years ago
#5133 closed enhancement (fixed)
[with patch, positive review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
Reported by: | was | Owned by: | burcin |
---|---|---|---|
Priority: | major | Milestone: | sage-4.1.1 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | Sage 4.1.1.alpha1 | |
Authors: | Burcin Erocal | Reviewers: | Martin Albrecht |
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
wstein@sage:~/build/sage-3.2.3/devel/sage/sage/ext$ sage -coverage multi_modular.pyx ---------------------------------------------------------------------- multi_modular.pyx ERROR: Please define a s == loads(dumps(s)) doctest. SCORE multi_modular.pyx: 0% (0 of 15) Missing documentation: * _extend_moduli_to_height(self, height): * _extend_moduli(self, count): * precomputation_list(self): * partial_product(self, n): * prod(self): * list(self): * __len__(self): * __iter__(self): * __getitem__(self, ix): * __repr__(self): * next_prime(self): * replace_prime(self, ix): Missing doctests: * __init__(self, height): * _extend_moduli_to_count(self, int count): * crt(self, b): ---------------------------------------------------------------------- wstein@sage:~/build/sage-3.2.3/devel/sage/sage/ext$
Attachments (4)
Change History (15)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Owner changed from was to burcin
- Status changed from new to assigned
- Summary changed from improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
attachment:trac_5133-multi_modular_tests.patch adds doctests and fixes some possible segfaults by reorganizing the memory allocations in sage.ext.multi_modular
. Changes introduced by the patch are:
- 100% coverage
- refactor memory management
- use random primes
- set upper/lower bounds for moduli on initialization
comment:2 follow-up: ↓ 3 Changed 10 years ago by
- Summary changed from [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, needs work] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
Review
- typo:
l_bound
->u_bound
in docstring - the string representation is quite unusual, not sure if that's desired.
isinstance(val, list)
should allow more typesisinstance(val, (list,tuple,GeneratorType))
- Why is it okay to use
proof=False
for random integers, because they are so small anyway? That should be added as a comment. __cmp__
wants you to return -1,0,1 and notTrue
orFalse
, cf. http://docs.python.org/reference/datamodel.htmlINPUT::
->INPUT:
, it doesn't get two : because the following block is not literal- patch applies cleanly against 4.1
- doctest failures:
sage -t devel/sage/sage/modular/modsym/heilbronn.pyx # 1 doctests failed sage -t devel/sage/sage/modular/modsym/subspace.py # 12 doctests failed sage -t devel/sage/sage/modular/modsym/space.py # 9 doctests failed sage -t devel/sage/sage/modular/modform/eisenstein_submodule.py # 2 doctests failed sage -t devel/sage/sage/modular/modform/space.py # 3 doctests failed sage -t devel/sage/sage/modular/hecke/degenmap.py # 1 doctests failed sage -t devel/sage/doc/en/bordeaux_2008/elliptic_curves.rst # 5 doctests failed sage -t devel/sage/sage/modular/modsym/ambient.py # 3 doctests failed sage -t devel/sage/sage/modular/hecke/element.py # 1 doctests failed sage -t devel/sage/sage/modular/hecke/hecke_operator.py # 1 doctests failed sage -t devel/sage/sage/modular/abvar/homology.py # 4 doctests failed sage -t devel/sage/sage/modular/abvar/morphism.py # 1 doctests failed sage -t devel/sage/sage/modular/hecke/module.py # 3 doctests failed sage -t devel/sage/sage/modular/abvar/torsion_subgroup.py # 4 doctests failed sage -t devel/sage/sage/modular/hecke/submodule.py # 4 doctests failed sage -t devel/sage/sage/modular/abvar/abvar.py # 10 doctests failed sage -t devel/sage/sage/modular/modform/element.py # 1 doctests failed sage -t devel/sage/sage/modular/abvar/homspace.py # 4 doctests failed sage -t devel/sage/sage/combinat/symmetric_group_representations.py # 1 doctests failed sage -t devel/sage/sage/geometry/lattice_polytope.py # 15 doctests failed sage -t devel/sage/sage/schemes/elliptic_curves/padic_lseries.py # 12 doctests failed sage -t devel/sage/sage/schemes/elliptic_curves/sha_tate.py # 6 doctests failed sage -t devel/sage/sage/schemes/elliptic_curves/ell_modular_symbols.py # 4 doctests failed
comment:3 in reply to: ↑ 2 Changed 10 years ago by
- Summary changed from [with patch, needs work] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
Replying to malb:
- typo:
l_bound
->u_bound
in docstring
Fixed.
- the string representation is quite unusual, not sure if that's desired.
Changed it to:
sage: from sage.ext.multi_modular import MultiModularBasis_base sage: mm = MultiModularBasis_base([3, 5, 7]); mm MultiModularBasis with moduli [3, 5, 7]
isinstance(val, list)
should allow more typesisinstance(val, (list,tuple,GeneratorType))
Done.
- Why is it okay to use
proof=False
for random integers, because they are so small anyway? That should be added as a comment.
I'd forgotten that I put that in. Good catch. I removed that argument, so we use the global flag now.
__cmp__
wants you to return -1,0,1 and notTrue
orFalse
, cf. http://docs.python.org/reference/datamodel.html
Fixed. Who would compare these things anyway? :)
INPUT::
->INPUT:
, it doesn't get two : because the following block is not literal
Fixed.
- patch applies cleanly against 4.1
- doctest failures:
Doh! I only doctested the sage/matrix
directory. Apparently the only user of MultiModularBasis
, namely matrix_integer_dense.pyx
didn't have doctests for it. Now it does.
comment:4 Changed 10 years ago by
- Summary changed from [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, positive review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
all tests pass, issues were addressed.
comment:5 Changed 10 years ago by
- Summary changed from [with patch, positive review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
I got a hunk failure when applying trac_5133-multi_modular_tests-take2.patch
:
[mvngu@sage sage-exp]$ hg qimport http://trac.sagemath.org/sage_trac/raw-attachment/ticket/5133/trac_5133-multi_modular_tests-take2.patch adding trac_5133-multi_modular_tests-take2.patch to series file [mvngu@sage sage-exp]$ hg qpush -a applying trac_5133-multi_modular_tests-take2.patch patching file sage/rings/arith.py Hunk #2 FAILED at 914 1 out of 4 hunks FAILED -- saving rejects to file sage/rings/arith.py.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir Errors during apply, please fix and refresh trac_5133-multi_modular_tests-take2.patch
Here's the hunk that failed:
--- arith.py +++ arith.py @@ -915,6 +915,9 @@ the function uses a pseudo-primality test, which is much faster for really big numbers but does not provide a proof of primality. If None, uses the global default (see sage.structure.proof) + + - ``lbound`` - an integer >= 2 + lower bound for the chosen primes EXAMPLES::
This is because I previously applied the patches at #6529. The failure has been manually resolved so the docstring of random_prime
in sage/rings/arith.py
now reads:
Returns a random prime p between `lbound` and n (i.e. `lbound <= p <= n`). The returned prime is chosen uniformly at random from the set of prime numbers less than or equal to n. INPUT: - ``n`` - an integer >= 2. - ``proof`` - bool or None (default: None) If False, the function uses a pseudo-primality test, which is much faster for really big numbers but does not provide a proof of primality. If None, uses the global default (see :mod:`sage.structure.proof.proof`) - ``lbound`` - an integer >= 2 lower bound for the chosen primes
The patch trac_5133-take2-rebased.patch
is a rebase of trac_5133-multi_modular_tests-take2.patch
that depends on first applying the patches at #6529.
comment:6 Changed 10 years ago by
- Reviewers set to Martin Albrecht
- Summary changed from [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, positive review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
comment:7 Changed 10 years ago by
- Summary changed from [with patch, positive review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
Oops... can I get someone to do a quick review of the rebased patch?
comment:8 Changed 10 years ago by
- Summary changed from [with patch, needs review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable to [with patch, positive review] improve the coverage of ext/multi_modular.pyx from an abysmal 0% to something more respectable
I changed the error message random_prime()
gives when lower_bound
is < n
.
Positive review for the rebased patch.
comment:9 Changed 10 years ago by
- Merged in set to sage-4.1.1.alpha0
- Resolution set to fixed
- Status changed from assigned to closed
comment:10 Changed 10 years ago by
- Merged in sage-4.1.1.alpha0 deleted
- Resolution fixed deleted
- Status changed from closed to reopened
This will have to wait for Sage 4.1.1.alpha1. Apparently, I accidentally closed this as being merged in 4.1.1.alpha0.
comment:11 Changed 10 years ago by
- Merged in set to Sage 4.1.1.alpha1
- Resolution set to fixed
- Status changed from reopened to closed
Merged trac_5133-take2-rebased2.patch
.
add doctests and fix memory management in sage.ext.multi_modular