Opened 11 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)

trac_5133-multi_modular_tests.patch (37.5 KB) - added by burcin 10 years ago.
add doctests and fix memory management in sage.ext.multi_modular
trac_5133-multi_modular_tests-take2.patch (39.2 KB) - added by burcin 10 years ago.
second try, apply only this one
trac_5133-take2-rebased.patch (39.2 KB) - added by mvngu 10 years ago.
rebased to depend on #6529
trac_5133-take2-rebased2.patch (39.3 KB) - added by burcin 10 years ago.
changed error message in rebased patch

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by burcin

add doctests and fix memory management in sage.ext.multi_modular

comment:1 Changed 10 years ago by burcin

  • Authors set to Burcin Erocal
  • 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: Changed 10 years ago by malb

  • 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 types isinstance(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 not True or False, cf. http://docs.python.org/reference/datamodel.html
  • INPUT:: -> 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 burcin

  • 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 types isinstance(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.

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.

Changed 10 years ago by burcin

second try, apply only this one

comment:4 Changed 10 years ago by malb

  • 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.

Changed 10 years ago by mvngu

rebased to depend on #6529

comment:5 Changed 10 years ago by mvngu

  • 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 mvngu

  • 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 mvngu

  • 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?

Changed 10 years ago by burcin

changed error message in rebased patch

comment:8 Changed 10 years ago by burcin

  • 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 mvngu

  • 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 mvngu

  • 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 mvngu

  • 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.

Note: See TracTickets for help on using tickets.