Opened 12 years ago
Closed 15 months ago
#9777 closed enhancement (fixed)
include more rings in random testing
Reported by:  Niles Johnson  Owned by:  Alex Ghitza 

Priority:  major  Milestone:  sage9.5 
Component:  algebra  Keywords:  random testing, rings 
Cc:  Travis Scrimshaw, Samuel Lelièvre, ghkliem  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Michael Orlitzky 
Report Upstream:  N/A  Work issues:  
Branch:  8df28fa (Commits, GitHub, GitLab)  Commit:  8df28fa8b88b8e518f156a770d20a322d3028961 
Dependencies:  Stopgaps: 
Description
padics should be included, perhaps at "level 0"?
The following "level 1" rings are not included in sage.rings.tests
:
 power series rings
 Laurent series rings
 multivariate power series rings (implemented in #1956)
 infinite polynomial rings
Also, it's not clear that "level n" testing occurs for n > 1
; e.g. multivariate polynomial ring in 3 variables over Laurent series ring over finite field of size 29, etc.
Quotient rings are also not included, but should be. There are probably more to be included than this list indicates.
Change History (13)
comment:1 Changed 16 months ago by
Authors:  → Frédéric Chapoton 

Branch:  → public/ticket/9777 
Commit:  → 1d8ef50b594b67e72f7950a0ba44a99054452b42 
Status:  new → needs_review 
comment:2 Changed 15 months ago by
Cc:  Travis Scrimshaw Samuel Lelièvre ghkliem added 

any opinion on the pertinence ?
comment:3 Changed 15 months ago by
Cc:  Michael Orlitzky added 

Is this doctest checking the right function?
+
+def padic_field():
+ """
+ Return a random padic field modulo n with p at most 10000
+ and precision between 10 and 100.
+
+ EXAMPLES::
+
+ sage: import sage.rings.tests
+ sage: sage.rings.tests.integer_mod_ring()
+ Ring of integers modulo 30029
+ """
+ from sage.all import ZZ, Qp
+ prec = ZZ.random_element(x=10, y=100)
+ p = ZZ.random_element(x=2, y=10**4  30).next_prime()
+ return Qp(p, prec)
+
+
In any case, it would be nice to avoid adding new tests that require a specific random seed. We're pretty close to making all testing random testing.
comment:4 Changed 15 months ago by
Cc:  Michael Orlitzky removed 

I am not sure how useful this is for catching bugs as we do (or at least should do) good test coverage within the individual files/rings themselves. However, I would follow @mjo's recommendation as I don't have a strong opinion on this.
comment:5 Changed 15 months ago by
Commit:  1d8ef50b594b67e72f7950a0ba44a99054452b42 → 63fa6a3adb1e1f4bd6b116f299e83d531c68bc93 

comment:7 Changed 15 months ago by
Reviewers:  → Michael Orlitzky 

Status:  needs_review → positive_review 
Thanks! Many of these tests can probably be removed later on, but it's nice to have this old ticket fixed in the meantime, especially in the likely event that we all forget about it for another decade.
comment:8 Changed 15 months ago by
Milestone:  → sage9.5 

comment:10 Changed 15 months ago by
Commit:  63fa6a3adb1e1f4bd6b116f299e83d531c68bc93 → 8df28fa8b88b8e518f156a770d20a322d3028961 

Branch pushed to git repo; I updated commit sha1. New commits:
8df28fa  Merge branch 'public/ticket/9777' in 9.5.b0

comment:11 Changed 15 months ago by
Status:  needs_work → needs_review 

indeed a serious merge conflict. Needs another round of review, please
comment:13 Changed 15 months ago by
Branch:  public/ticket/9777 → 8df28fa8b88b8e518f156a770d20a322d3028961 

Resolution:  → fixed 
Status:  positive_review → closed 
Here is a branch.
But is this still pertinent ?
New commits:
more tests for rings