Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include more rings in random testing #9777

Closed
nilesjohnson mannequin opened this issue Aug 21, 2010 · 18 comments
Closed

include more rings in random testing #9777

nilesjohnson mannequin opened this issue Aug 21, 2010 · 18 comments

Comments

@nilesjohnson
Copy link
Mannequin

nilesjohnson mannequin commented Aug 21, 2010

p-adics should be included, perhaps at "level 0"?

The following "level 1" rings are not included in sage.rings.tests:

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.

CC: @tscrim @slel @kliem

Component: algebra

Keywords: random testing, rings

Author: Frédéric Chapoton

Branch/Commit: 8df28fa

Reviewer: Michael Orlitzky

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

@nilesjohnson nilesjohnson mannequin added c: algebra labels Aug 21, 2010
@nilesjohnson nilesjohnson mannequin assigned aghitza Aug 21, 2010
@fchapoton
Copy link
Contributor

comment:1

Here is a branch.

But is this still pertinent ?


New commits:

1d8ef50more tests for rings

@fchapoton
Copy link
Contributor

Author: Frédéric Chapoton

@fchapoton
Copy link
Contributor

Commit: 1d8ef50

@fchapoton
Copy link
Contributor

Branch: public/ticket/9777

@fchapoton
Copy link
Contributor

comment:2

any opinion on the pertinence ?

@orlitzky
Copy link
Contributor

comment:3

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.

@tscrim
Copy link
Collaborator

tscrim commented Aug 20, 2021

comment:4

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 @orlitzky's recommendation as I don't have a strong opinion on this.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2021

Changed commit from 1d8ef50 to 63fa6a3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2021

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

@fchapoton
Copy link
Contributor

comment:6

ok, should be better now

@orlitzky
Copy link
Contributor

Reviewer: Michael Orlitzky

@orlitzky
Copy link
Contributor

comment:7

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.

@vbraun
Copy link
Member

vbraun commented Aug 29, 2021

comment:9

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2021

Changed commit from 63fa6a3 to 8df28fa

@fchapoton
Copy link
Contributor

comment:11

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

@orlitzky
Copy link
Contributor

orlitzky commented Sep 2, 2021

comment:12

Still OK.

@vbraun
Copy link
Member

vbraun commented Sep 7, 2021

Changed branch from public/ticket/9777 to 8df28fa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants