Make modular doctests ready for random seeds

[kliem]

This ticket makes

sage -t --long --random-seed=n src/sage/modular/

pass for different values n than just 0.

Depends on #32124

CC: @dimpase

Component: doctest framework

Author: Jonathan Kliem

Branch/Commit: a890c4d

Reviewer: Dima Pasechnik

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

[kliem]

comment:1

I have a partial fix. After that at least the following need fixing

sage -t --long --random-seed=151058820726654196682836430928254760259 src/sage/modular/arithgroup/arithgroup_perm.py  # 5 doctests failed
sage -t --long --random-seed=151058820726654196682836430928254760259 src/sage/modular/arithgroup/congroup_sl2z.py  # 3 doctests failed
sage -t --long --random-seed=151058820726654196682836430928254760259 src/sage/modular/dirichlet.py  # 2 doctests failed
sage -t --long --random-seed=151058820726654196682836430928254760259 src/sage/modular/hecke/algebra.py  # 1 doctest failed
sage -t --long --random-seed=151058820726654196682836430928254760259 src/sage/modular/modform/numerical.py  # 2 doctests failed
sage -t --long --random-seed=151058820726654196682836430928254760259 src/sage/modular/overconvergent/hecke_series.py  # 1 doctest failed

[kliem]

Dependencies: #29962

[kliem]

Commit: 48456b4

[kliem]

Branch: public/29977

[kliem]

New commits:

da1c6be	start from a "random" random seed for doctesting
b7b836d	make random seed reproducible
eedbe5e	document random_seed
998b1b9	default random seed 0 for now
1d7b00e	dash instead of underscore for command line options
48456b4	partially make modular fuzz ready

[mkoeppe]

comment:5

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[sagetrac-git]

Changed commit from 48456b4 to 20365ae

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

20365ae

make modular ready for fuzzed doctests

[kliem]

Author: Jonathan Kliem

[kliem]

Changed dependencies from #29962 to none

[slel]

comment:9

Continuing the discussion from #30801 comment:202
and following comments here.

One way to get compiler independent output
would be to sort according to string representation.

[kliem]

comment:10

That is actually a super easy fix.

[sagetrac-git]

Changed commit from 20365ae to 54b325e

[sagetrac-git]

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

54b325e

sort groups of smooth characters by string for stable doctesting

[sagetrac-tmonteil]

comment:12

I came here from patchbots failure in src/sage/modular/local_comp/local_comp.py where the issue probably comes from the use of sets, so prehaps sorting the list is not needed, and it makes the examples less understandable. Did you try to just replace set(Pi.characters()) with Pi.characters() instead of sorted(Pi.characters(), key=str) ?

[kliem]

comment:13

I don't know if it is stable. Dima decided to use sets for those, so I figured the order can be random.

[kliem]

comment:14

@dimpase: Is there a reason you put the set around Pi.characters? Is the order unstable?

[dimpase]

comment:15

I made these changes to fix failing doctests. And it seemed to work. I have no idea why it didn't work in the end

[sagetrac-tmonteil]

comment:16

I bet that the ordering changed with Pari upgrade, but it might be stable for a given Pari release.

[kliem]

comment:17

It's a problem of our api, I believe. sorted(list(Pi.characters())) doesn't do anything. This is how the doctests output sets and if sorting doesn't change the order, then it won't work.

[dimpase]

comment:18

after pari 2.13 got positively reviewed, but not yet merged, a long coming #26635 got merged, and spoilt tests. So I tried to fix them.

[sagetrac-tmonteil]

comment:20

Replying to @kliem:

I don't blame you. I'm just trying to explain why doctesting sets works usually fine, but not in this case. I also added you, to figure out, whether we need sorting at all. But apparently yes and apparently sorting by strings is an easy solution for now.

Do you have some evidence that we have to sort the lists ? The problem with sorting is that examples are not easy to understand for users (usually not involved in doctesting).

[kliem]

comment:21

Other than #30801 comment:206 I don't have any evidence.

Pi.characters() is stable on my machines. But set(Pi.characters()) isn't. I'm asking you, whether there is a reason to use set instead of just leaving it. I'm assuming there is, because Dima changed it.

If Pi.characters() doesn't work, we need to either find a workaround (like sort by strings) or fix sorting.

[dimpase]

comment:22

on my machines, ironically, set() was stable.

[kliem]

comment:23

Yes, it is stable within the machine, because apparently the hash is stable (the order of the hash to be precise). That is the pretty print order of a set, if sorted does not do anything.

On my patchbot it is stable, but the other way around. So the patchbot is down until this is fixed (the problem was the whole issue with setuptools_scm which lead to the patchbot being down in the first place and now all tests need to pass).

[sagetrac-git]

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

e6a697a	truely ignore ignored bounds for ZZ.random_element
902f851	Merge branch 'public/32124' of git://trac.sagemath.org/sage into public/29977

[sagetrac-git]

Changed commit from 54b325e to 902f851

[kliem]

Dependencies: #32124

[slel]

comment:26

Replying to @sagetrac-tmonteil:

I came here from patchbots failure in src/sage/modular/local_comp/local_comp.py
where the issue probably comes from the use of sets, so perhaps
sorting the list is not needed, and it makes the examples less
understandable. Did you try to just replace set(Pi.characters())
with Pi.characters() instead of sorted(Pi.characters(), key=str)?

See comments #30801 comment:202 and following.

Sorting by string does obscure the example a bit
but makes doctests pass with minimal disruption.

The following avoids sorting by string
but is a more disruptive change:

        A more complicated example (higher weight and nontrivial central character)::

             sage: f = Newforms(GammaH(25, [6]), 3, names='j')[0]; f
             q + j0*q^2 + 1/3*j0^3*q^3 - 1/3*j0^2*q^4 + O(q^6)
             sage: Pi = LocalComponent(f, 5)
-            sage: set(Pi.characters())
-            {Character of unramified extension Q_5(s)* (s^2 + 4*s + 2 = 0), of level 1, mapping s |--> 1/3*j0^2*d - 1/3*j0^3, 5 |--> 5,
-             Character of unramified extension Q_5(s)* (s^2 + 4*s + 2 = 0), of level 1, mapping s |--> -1/3*j0^2*d, 5 |--> 5}
-            sage: Pi.characters()[0].base_ring()
-            Number Field in d with defining polynomial x^2 - j0*x + 1/3*j0^2 over its base field
+            sage: chars = Pi.characters()
+            sage: len(chars)
+            2
+            sage: a, b = chars
+            sage: a.galois_conjugate() == b
+            True
+            sage: C = a.parent()
+            sage: C
+            Group of smooth characters
+              of unramified extension Q_5(s)* (s^2 + 4*s + 2 = 0)
+              with values in Number Field in d
+              with defining polynomial x^2 - j0*x + 1/3*j0^2 over its base field
+            sage: d = a.base_ring().gen()
+            sage: s = C.number_field().gen()
+            sage: j0 = a.base_ring().relative_polynomial().base_ring().gen()
+            sage: a(5) == b(5) == 5
+            True
+            sage: {a(s), b(s)} == {(1/3*j0^2*d - 1/3*j0^3), (-1/3*j0^2*d)}
+            True

Another option would be to implement
a platform-independent sorting of characters.

[dimpase]

comment:27

is there a way to efficiently check that the characters in question are characters, and their set is exhaustive in some sense?

anyhow the test for len(chars) above may be skipped.

[sagetrac-tmonteil]

comment:28

I created #32134 for the src/sage/modular/local_comp/local_comp.py issue, as it is not related to the random seed (which is the purpose of this ticket).

My bet is that the order changed due to Pari upgrade, but it is constant for a given Pari version, hence we can use raw lists, not sets, nor sorting, nor cumbersome doctests.

Could you please try that branch, and see if it passes for all of you.

[dimpase]

comment:29

did you try 32-bit as well as 64 bit?

[kliem]

comment:30

Indeed I added this issue to the ticket here because I mistakenly figured it was connected to random seeds. Whatever makes this issue get a positive review quickest, is what I would prefer. So a seperate ticket sounds indeed fine.

I agree with Dima that only because it passes on a few computers, it doesn't tell you anything about general stability.

[sagetrac-tmonteil]

comment:31

Replying to @kliem:

I agree with Dima that only because it passes on a few computers, it doesn't tell you anything about general stability.

What i am lookinkg for is whether there was an issue on a single computer when we use lists instead of sets (i could not find any evidence in the various discussions).

Replying to @dimpase:

did you try 32-bit as well as 64 bit?

Unfortunately, i currently do not have any 32bit builder.

Let us collect results of various builds on #32134.

[sagetrac-tmonteil]

comment:32

If the behaviour depends on the architecture, we can use the # 64-bit and # 32-bit tags.

[kliem]

comment:33

I much rather have some fix soon and a nice fix later.

We only have 3 patchbots at the moment and 3 are down exclusively because of this issue (I suspect one more that also had some interrupt).

[sagetrac-git]

Changed commit from 902f851 to a890c4d

[sagetrac-git]

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

a890c4d

one more test from modular

[dimpase]

comment:35

lgtm

[dimpase]

Reviewer: Dima Pasechnik

[kliem]

comment:36

Thank you.

[sagetrac-tmonteil]

comment:37

Setting this to blocker so that we can restart patchbots.

[vbraun]

Changed branch from public/29977 to a890c4d