Make modular doctests ready for random seeds
kliem opened this issue · 44 comments
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
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
Branch: public/29977
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
20365ae | make modular ready for fuzzed doctests |
Author: Jonathan Kliem
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.
That is actually a super easy fix.
Branch pushed to git repo; I updated commit sha1. New commits:
54b325e | sort groups of smooth characters by string for stable doctesting |
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) ?
I don't know if it is stable. Dima decided to use sets for those, so I figured the order can be random.
@dimpase: Is there a reason you put the set around Pi.characters? Is the order unstable?
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
I bet that the ordering changed with Pari upgrade, but it might be stable for a given Pari release.
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.
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.
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.
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).
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.
on my machines, ironically, set() was stable.
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).
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 replaceset(Pi.characters())
withPi.characters()instead ofsorted(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)}
+ TrueAnother option would be to implement
a platform-independent sorting of characters.
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.
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.
did you try 32-bit as well as 64 bit?
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.
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.
If the behaviour depends on the architecture, we can use the # 64-bit and # 32-bit tags.
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).
Branch pushed to git repo; I updated commit sha1. New commits:
a890c4d | one more test from modular |
lgtm
Reviewer: Dima Pasechnik
Thank you.
Setting this to blocker so that we can restart patchbots.
Changed branch from public/29977 to a890c4d