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: sage-9.5
Component: algebra Keywords: random testing, rings
Cc: Travis Scrimshaw, Samuel Lelièvre, gh-kliem 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:

Status badges

Description

p-adics 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 Frédéric Chapoton

Authors: Frédéric Chapoton
Branch: public/ticket/9777
Commit: 1d8ef50b594b67e72f7950a0ba44a99054452b42
Status: newneeds_review

Here is a branch.

But is this still pertinent ?


New commits:

1d8ef50more tests for rings

comment:2 Changed 15 months ago by Frédéric Chapoton

Cc: Travis Scrimshaw Samuel Lelièvre gh-kliem added

any opinion on the pertinence ?

comment:3 Changed 15 months ago by Michael Orlitzky

Cc: Michael Orlitzky added

Is this doctest checking the right function?

+
+def padic_field():
+    """
+    Return a random p-adic 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 Travis Scrimshaw

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 git

Commit: 1d8ef50b594b67e72f7950a0ba44a99054452b4263fa6a3adb1e1f4bd6b116f299e83d531c68bc93

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

5d4db0aMerge branch 'public/ticket/9777' in 9.4.rc2
63fa6a3fix some doctests, and harden them

comment:6 Changed 15 months ago by Frédéric Chapoton

ok, should be better now

comment:7 Changed 15 months ago by Michael Orlitzky

Reviewers: Michael Orlitzky
Status: needs_reviewpositive_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 Frédéric Chapoton

Milestone: sage-9.5

comment:9 Changed 15 months ago by Volker Braun

Status: positive_reviewneeds_work

Merge conflict

comment:10 Changed 15 months ago by git

Commit: 63fa6a3adb1e1f4bd6b116f299e83d531c68bc938df28fa8b88b8e518f156a770d20a322d3028961

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

8df28faMerge branch 'public/ticket/9777' in 9.5.b0

comment:11 Changed 15 months ago by Frédéric Chapoton

Status: needs_workneeds_review

indeed a serious merge conflict. Needs another round of review, please

comment:12 Changed 15 months ago by Michael Orlitzky

Status: needs_reviewpositive_review

Still OK.

comment:13 Changed 15 months ago by Volker Braun

Branch: public/ticket/97778df28fa8b88b8e518f156a770d20a322d3028961
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.