Opened 2 years ago
Closed 2 years ago
#23671 closed enhancement (fixed)
hypergeometric motives
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  modular forms  Keywords:  hypergeometric motives, Euler factors, sd91 
Cc:  kedlaya, jvoight, tornaria  Merged in:  
Authors:  Frédéric Chapoton, Kiran Kedlaya  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  d8c961b (Commits)  Commit:  d8c961bfe6d67c265a5c615557ad7744cdf42e20 
Dependencies:  Stopgaps: 
Description (last modified by )
For the moment, a first sketch.
The computation of Euler factors (via traces of Frobenius) is only for good primes.
Change History (102)
comment:1 Changed 2 years ago by
 Branch set to u/chapoton/23671
 Commit set to 8ce1dc8c6e40300c1929d814048d79e19e146745
 Dependencies set to #23585
 Status changed from new to needs_info
comment:2 Changed 2 years ago by
 Commit changed from 8ce1dc8c6e40300c1929d814048d79e19e146745 to 9a281db9be4ca4bfda004aff12ea1610cfb04135
comment:3 Changed 2 years ago by
 Commit changed from 9a281db9be4ca4bfda004aff12ea1610cfb04135 to c123aeae45c2e428d49d9f6a2a60adaf51cd6053
comment:4 Changed 2 years ago by
 Commit changed from c123aeae45c2e428d49d9f6a2a60adaf51cd6053 to b8428bc7a8f27ad680497fb7c8c0b41bd1fd20cb
comment:5 Changed 2 years ago by
 Commit changed from b8428bc7a8f27ad680497fb7c8c0b41bd1fd20cb to 09bf0d0c94747950f312d28ba79845b161c8eaa1
Branch pushed to git repo; I updated commit sha1. New commits:
09bf0d0  trac 23671 insertion in the doc

comment:6 Changed 2 years ago by
I'm alerting the LMFDB people who work on hypergeometric motives to the existence of this code.
comment:7 Changed 2 years ago by
Thanks. Maybe they can help me to fix the lurking bug.
comment:8 Changed 2 years ago by
 Commit changed from 09bf0d0c94747950f312d28ba79845b161c8eaa1 to bec14e0c19e91137d0d5fbec9e821d9a274c29c5
Branch pushed to git repo; I updated commit sha1. New commits:
bec14e0  trac 23671 one detail and more doctests

comment:9 Changed 2 years ago by
 Cc kedlaya added
comment:10 Changed 2 years ago by
 Commit changed from bec14e0c19e91137d0d5fbec9e821d9a274c29c5 to b2bba5d0227ff70175745ccda638add7a7f629cf
Branch pushed to git repo; I updated commit sha1. New commits:
b2bba5d  trac 23585 many details

comment:11 Changed 2 years ago by
 Commit changed from b2bba5d0227ff70175745ccda638add7a7f629cf to 1005e0aff6f1d873b21c7b5882a1557612f09b56
Branch pushed to git repo; I updated commit sha1. New commits:
1005e0a  trac 23671 detail

comment:12 Changed 2 years ago by
 Commit changed from 1005e0aff6f1d873b21c7b5882a1557612f09b56 to b40dd7d1db23f975baf054802d61f5d34b1f5164
Branch pushed to git repo; I updated commit sha1. New commits:
b40dd7d  trac 23671 method swap_alpha_beta

comment:13 Changed 2 years ago by
 Commit changed from b40dd7d1db23f975baf054802d61f5d34b1f5164 to 6dc9bc1b4b8b94b90c6c238c957378072f5c45f1
Branch pushed to git repo; I updated commit sha1. New commits:
6dc9bc1  trac 23671 more helpful message

comment:14 Changed 2 years ago by
 Commit changed from 6dc9bc1b4b8b94b90c6c238c957378072f5c45f1 to f4190ab33e2802adefd6f078153dbeba9b99d672
Branch pushed to git repo; I updated commit sha1. New commits:
f4190ab  trac 23671 detail (comment)

comment:15 Changed 2 years ago by
 Commit changed from f4190ab33e2802adefd6f078153dbeba9b99d672 to 7d8590c6b9e2f260997c939c8f624b6e5b0a23cd
Branch pushed to git repo; I updated commit sha1. New commits:
7d8590c  care for doc

comment:16 Changed 2 years ago by
 Commit changed from 7d8590c6b9e2f260997c939c8f624b6e5b0a23cd to 5f1be177550319a13402d928c2605facd4e78c64
Branch pushed to git repo; I updated commit sha1. New commits:
5f1be17  details

comment:17 Changed 2 years ago by
Is Hyp
a temporary name? Magma's analogue is HypergeometricData
, which would be reasonable to use here also.
comment:18 Changed 2 years ago by
yes, indeed. But as long as the euler_factor
method is not working correctly, all this has little value..
comment:19 Changed 2 years ago by
Is this a correct illustration of the issue with Gauss sums?
sage: H = Hyp(gamma_list=[6,1,4,3]) sage: t = 189/125 sage: H.H_value(19,1,t) 0 sage: H.padic_H_value(19,1,t) # should give the same answer? 3
comment:20 Changed 2 years ago by
Yes, this is typical. Either our padic Gauss sum has a problem, or my code has a problem. I wrote very parallel code for H_value and padic_H_value, but this does not prove that they are both correct. The H_value method seems to be working rather well, compared to the literature.
comment:21 Changed 2 years ago by
I don't see any problem with the padic Gauss sums. Here is a test:
sage: def f(z,p): ....: return z  (z^p1)/(p*z^(p1)) sage: from sage.arith.misc import gauss_sum sage: from sage.rings.padics.misc import gauss_sum as padic_gauss_sum sage: p = 19 sage: R = padic_gauss_sum(1, p, 1, 10).parent() sage: z = 1  R.gen() sage: for i in range(10): z = f(z,p) sage: z *= R.teichmuller(GF(p).multiplicative_generator()) sage: S.<a> = CyclotomicField(p*(p1)) sage: for i in range(p1): ....: t = gauss_sum(S.zeta(p1)^i, GF(p)).polynomial()(z) ....: u = padic_gauss_sum(i, p, 1, 10) ....: print (t  u).is_zero()
returns all True
.
I'm guessing the issue is the normalization of the comparison between abstract and padic roots of unity, which is critical to the GaussKoblitz formula. I haven't traced this fully, but it looks like in the lines:
prod(padic_gauss_sum(xp * r, p, f, prec) for xp in gamma_pos) / prod(padic_gauss_sum(xq * r, p, f, prec) for xq in gamma_neg) *
if one changes xp * r
to xp * r
and xq * r
to xq * r
, then the most egregious doctest failures in euler_factor
go away. (There remain cases where there is an unresolved sign ambiguity, which I need to stare at some more.)
comment:22 followup: ↓ 23 Changed 2 years ago by
Regarding the sign ambiguity: according to the Magma documentation, they are using the local functional equation to expedite the computation. That suggests that they have a formula for the sign of the functional equation, but I'm not sure where that is writen down.
In other news, I think the statement of the GrossKoblitz formula in the docstring of the padic Gauss sum is missing an overall minus sign, which is in Robert's paper and also in the code.
comment:23 in reply to: ↑ 22 Changed 2 years ago by
Replying to kedlaya:
Regarding the sign ambiguity: according to the Magma documentation, they are using the local functional equation to expedite the computation. That suggests that they have a formula for the sign of the functional equation, but I'm not sure where that is writen down.
According to Mark Watkins, Magma uses the formulas given in 11.1.1 of http://magma.maths.usyd.edu.au/~watkins/papers/known.pdf for the sign of the local functional equation at good primes. (This is for even weight; for odd weight the sign is always +1.)
comment:24 Changed 2 years ago by
Great ! Thank you for investigating. I will now correct that.
comment:25 Changed 2 years ago by
 Commit changed from 5f1be177550319a13402d928c2605facd4e78c64 to 445de286f6f8c64eaf0e9debd53440fc800c60bc
comment:26 Changed 2 years ago by
 Commit changed from 445de286f6f8c64eaf0e9debd53440fc800c60bc to 9012cc56cb8eeb9ff1a04acb157e45f24879e607
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9012cc5  first sketch of implementation of hypergeometric motives

comment:27 Changed 2 years ago by
Humm, it seems that this does not fix all the doctests of padic_H_value
. Some of the doctests for (p,2,t) for the first HGM example are still wrong (by a factor of p).
comment:28 Changed 2 years ago by
 Commit changed from 9012cc56cb8eeb9ff1a04acb157e45f24879e607 to fd2f2efad4f61132a471bc0161e0701554a19b23
Branch pushed to git repo; I updated commit sha1. New commits:
fd2f2ef  remove comment

comment:29 Changed 2 years ago by
 Commit changed from fd2f2efad4f61132a471bc0161e0701554a19b23 to e1b5570f8384ef3f993f603d1c818bc8aa3c5666
Branch pushed to git repo; I updated commit sha1. New commits:
e1b5570  trac 23671 first step for ambiguityresolving

comment:30 Changed 2 years ago by
Suggestion: rather than defining Hyp
as as alias for HypergeometricMotive
, it might be safer to just change import Hyp
to import HypergeometricMotive as Hyp
in the doctests. (The abbreviation Hyp
could be misinterpreted in various ways, so I'm not sold on the wisdom of putting it into the global namespace.)
comment:31 Changed 2 years ago by
yes, for sure. But much more importantly we need to (1) fix the computation of padic_H_value and (2) resolve the ambiguity of Euler factors. I have no idea for (1) and plan to work on (2).
EDIT: And so far, nothing is added to the global namespace..
comment:32 Changed 2 years ago by
 Commit changed from e1b5570f8384ef3f993f603d1c818bc8aa3c5666 to 8e35f9a19b124179845183b012f00d2cdf5eebae
Branch pushed to git repo; I updated commit sha1. New commits:
8e35f9a  trac 23671 not setting Hyp as an alias

comment:33 Changed 2 years ago by
 Commit changed from 8e35f9a19b124179845183b012f00d2cdf5eebae to e76c51d92a4f2dc99ed64298d70d6eeb26a71b7b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e76c51d  first sketch of implementation of hypergeometric motives

comment:34 Changed 2 years ago by
 Commit changed from e76c51d92a4f2dc99ed64298d70d6eeb26a71b7b to 140dc5fb2662b90ca8a7d5c3f8eb6d3b85c581ef
Branch pushed to git repo; I updated commit sha1. New commits:
140dc5f  + one new method

comment:35 Changed 2 years ago by
I am going to take a look at the Magma code sometime soon to see if it sheds some light on the remaining doctest failures...
comment:36 Changed 2 years ago by
 Commit changed from 140dc5fb2662b90ca8a7d5c3f8eb6d3b85c581ef to 585257b6ab549a6de522f413067d14effbd6453e
Branch pushed to git repo; I updated commit sha1. New commits:
585257b  trac 23671 fixing Hyp calls

comment:37 Changed 2 years ago by
My only reference for the formula used in padic_H_value
is page 4 of http://magma.maths.usyd.edu.au/~watkins/papers/HGMchapter.pdf
comment:38 Changed 2 years ago by
 Commit changed from 585257b6ab549a6de522f413067d14effbd6453e to a6ac27e4fc508cf464bd7baeda6ec1a5ea09d3ff
comment:39 Changed 2 years ago by
 Commit changed from a6ac27e4fc508cf464bd7baeda6ec1a5ea09d3ff to babe4dbba281bd25c5d3341509d9c94e48e4f912
Branch pushed to git repo; I updated commit sha1. New commits:
babe4db  forgotten in change

comment:40 Changed 2 years ago by
 Commit changed from babe4dbba281bd25c5d3341509d9c94e48e4f912 to 916ec11f240223c46516c83902c6ea162036c39b
Branch pushed to git repo; I updated commit sha1. New commits:
916ec11  better handling of ComplexField H_value

comment:41 Changed 2 years ago by
In padic_H_value
, you might want to change
p_ring = Zp(p)
to
p_ring = Zp(p,prec=prec)
as otherwise the value of prec doesn't really make a difference in the internal computation. (This shouldn't matter to any of the current doctests, though.)
comment:42 Changed 2 years ago by
 Commit changed from 916ec11f240223c46516c83902c6ea162036c39b to 9d4e8398d3fcf633d59d58193320749108673aac
Branch pushed to git repo; I updated commit sha1. New commits:
9d4e839  trac 23671 detail

comment:43 followup: ↓ 47 Changed 2 years ago by
I think I found the remaining issue with padic_H_value
. The problem is actually in convert_to_Z
:
p_ring = x.parent().base_ring() p = p_ring.prime() L = x.list() print(p_ring, p, L) return ZZ.sum(L[(p  1) * i] * (p) ** i for i in range(1 + len(L) // (p  1)))
The trouble is that x.list()
behaves differently for an element of a padic ring (in which case it starts from 0) and an element of a padic field (in which case it starts from the valuation).
There are various ways to fix this. One would be to replace L = x.list()
with:
# Pass from field to ring so that list() starts from p^0. y = x.parent().integer_ring()(x) L = y.list()
to force the ring behavior.
comment:44 followup: ↓ 52 Changed 2 years ago by
In other news, there are some mathematical errors in the docstring for the padic Gauss sum. Maybe we should go ahead and fix them on this ticket:
Let `p` be a prime, `f` a positive integer, `q=p^f`, and `\pi` be the unique root of `f(x) = x^{p1}+p` congruent to `\zeta_p1` modulo `(\zeta_p1)^2`. Let `0\leq a < q1`. Then the GrossKoblitz formula gives us the value of the Gauss sum `g_q(a)` as a product of padic Gamma functions as follows: .. MATH:: g_q(a) = \pi^s \prod_{0\leq i < f} \Gamma_p(a^{(i)}/(q1)), where `s` is the sum of the digits of `a` in base `p` and the `a^{(i)}` have `p`adic expansions obtained from cyclic permutations of that of `a`.
comment:45 Changed 2 years ago by
 Commit changed from 9d4e8398d3fcf633d59d58193320749108673aac to 4f23442f652455ae381c7f923574a629746959be
Branch pushed to git repo; I updated commit sha1. New commits:
4f23442  trac 23671 fixing convert_to_Z

comment:46 Changed 2 years ago by
 Commit changed from 4f23442f652455ae381c7f923574a629746959be to dee34a4bd6fb9da44ddef80d92107019ea3b5592
Branch pushed to git repo; I updated commit sha1. New commits:
dee34a4  trac 23671 fixing doc of padic Gauss sums

comment:47 in reply to: ↑ 43 Changed 2 years ago by
Great ! Thanks a lot for finding that ! Now we can at last compute Euler factors for good primes. All tests pass.
Then we have 2 options. Trying to get his in sage in its present state, and enhance later. Or trying to go further before put this inside sage. I am not really planning to spend a lot more time on that, so I would prefer the first option.
Replying to kedlaya:
I think I found the remaining issue with
padic_H_value
. The problem is actually inconvert_to_Z
:p_ring = x.parent().base_ring() p = p_ring.prime() L = x.list() print(p_ring, p, L) return ZZ.sum(L[(p  1) * i] * (p) ** i for i in range(1 + len(L) // (p  1)))The trouble is that
x.list()
behaves differently for an element of a padic ring (in which case it starts from 0) and an element of a padic field (in which case it starts from the valuation).There are various ways to fix this. One would be to replace
L = x.list()
with:# Pass from field to ring so that list() starts from p^0. y = x.parent().integer_ring()(x) L = y.list()to force the ring behavior.
comment:48 Changed 2 years ago by
Definitely get this feature set into Sage first, then create separate tickets for additional features (like the Euler factors at bad reduction primes). This progress might serve as the basis for organizing a Sage Days on HGM sooner or later.
As soon as this is ready to go to needs_review
I will start looking more critically (particularly at the documentation, which so far I have mostly ignored).
comment:49 Changed 2 years ago by
 Description modified (diff)
comment:50 Changed 2 years ago by
 Commit changed from dee34a4bd6fb9da44ddef80d92107019ea3b5592 to 4856761ac805418cc7620ba688361f8e6e4cb206
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4856761  first sketch of implementation of hypergeometric motives

comment:51 Changed 2 years ago by
 Commit changed from 4856761ac805418cc7620ba688361f8e6e4cb206 to d5aae64066afbcb4edbc35d867b1122da8413c39
Branch pushed to git repo; I updated commit sha1. New commits:
d5aae64  trac 23671 adding tests

comment:52 in reply to: ↑ 44 Changed 2 years ago by
Replying to kedlaya:
In other news, there are some mathematical errors in the docstring for the padic Gauss sum. Maybe we should go ahead and fix them on this ticket:
That change has been made on #23780. The primary purpose of that ticket is to modify the padic Gauss sum computation to allow the caller to avoid instantiating the ramified extension of Q_p. With that, one can modify padic_H_value
to also avoid constructing the ramified extension, which in principle saves a factor of p
in complexity.
comment:53 Changed 2 years ago by
 Branch changed from u/chapoton/23671 to u/kedlaya/23671
comment:54 Changed 2 years ago by
 Commit changed from d5aae64066afbcb4edbc35d867b1122da8413c39 to db3363b4a150d7345e6ac1a8b5b72a426ccbf297
 Dependencies changed from #23585 to #23585, #23780
 Keywords hypergeometric motives Euler factors added
This commit implements my suggestion to do the computation entirely in Z_p by using the GaussKoblitz formula in factored form. Even saving a factor of p, it still seems to be much slower than Magma; not sure whether there is a single reason for that.
Last 10 new commits:
f5be269  trac 23585 doc detail

c1ca8da  trac 23585 allow to choose a character by its value on the mult. generator.

adeaa96  trac 23585 having finite_field as input

aea2dae  trac 23585 more doc (definition of Gauss sum)

67145d2  trac 23585 doc details, fine tuning

6fb031d  Merge branch 't/23585/gauss_sum_v2' into t/23671/23671

826ff2e  Modify padic Gauss sum to provide a factored option

50cd8bf  Fixed typo in doctest

eeecd24  Merge branch 't/23780/modify_p_adic_gauss_sum_to_allow_working_directly_in_q_p' into t/23671/23671

db3363b  Eliminate use of ramified extension

comment:55 Changed 2 years ago by
Fernando Rodriguez Villegas points out that Magma has built in a "wrong" (i.e., not consistent with standard references in the literature) sign convention, with the effect that the parameter values t and 1/t should be swapped. It's probably too late for Magma to change this, but it's not too late for Sage...
comment:56 Changed 2 years ago by
 Commit changed from db3363b4a150d7345e6ac1a8b5b72a426ccbf297 to cc19212f92fb3e1b005618e11016bf5683785a15
Branch pushed to git repo; I updated commit sha1. New commits:
8271a6b  first sketch of implementation of hypergeometric motives

a302f4d  trac 23671 adding tests

83aecd9  Modify padic Gauss sum to provide a factored option

700a0b7  Fixed typo in doctest

d57a3eb  Eliminate use of ramified extension

10377d0  Interchange t for 1/t

1b0c787  Merge branch 'u/kedlaya/23671' of git://trac.sagemath.org/sage into t/23671/23671

cc19212  General cleanup

comment:57 Changed 2 years ago by
 Commit changed from cc19212f92fb3e1b005618e11016bf5683785a15 to 9f5dd9dfa9ba042532b65125cd56c520530ef25e
Branch pushed to git repo; I updated commit sha1. New commits:
9f5dd9d  Add zigzag function, change class name to HypergeometricData

comment:58 Changed 2 years ago by
I changed the class name from HypergeometricMotive
to HypergeometricData
to match Magma. This is also conceptually better: this class represents a collection of combinatorial data corresponding to not a single motive, but a oneparameter family of motives indexed by t
.
comment:59 Changed 2 years ago by
Just to say that I agree with the name change. We could also have another class for objects where t has been chosen.
And I also agree with the switch t <> 1/t.
Thanks for working on all that !
comment:60 Changed 2 years ago by
 Commit changed from 9f5dd9dfa9ba042532b65125cd56c520530ef25e to 60c038b4ae556a0bd9b426f29dd1156f0d8b3507
Branch pushed to git repo; I updated commit sha1. New commits:
60c038b  Implement formula for sign of the functional equation

comment:61 Changed 2 years ago by
I implemented the explicit formula for the sign of the functional equation which I learned from Mark Watkins; this means that for degree d
, no more than d//2
traces are ever needed. (In particular, for d
odd this always saves one trace over the old method.)
In order to confirm that I did this correctly, I put in some more doctests. In particular, the doctests for euler_factor
cover all three cases of the functional equation (odd weight; even weight and odd degree; even weight and even degree).
I agree that later, we may want to add a class HypergeometricMotive
which includes the choice of t
, and which would have the Lseries as an attribute. Magma has a class for Lseries themselves, which has attributes like checking the functional equation and computing special values. However, that is an issue for a later ticket (as is adding the Euler factors for tame and wild primes).
This is actually looking pretty good now, to the extent that I'm planning to do a demonstration of this in a talk I'm giving at ICTP (Trieste) tomorrow (which was my motivation for making all these changes at this particular moment). Is it time for a review?
comment:62 Changed 2 years ago by
To answer my own question: no, this is not yet ready for review because I can still produce cases where it fails to agree with Magma (with some help from David Roberts). I'm guessing this has to do with the handling of the values 0 and 1 in the parameter lists; let me see if I can track it down.
comment:63 Changed 2 years ago by
 Commit changed from 60c038b4ae556a0bd9b426f29dd1156f0d8b3507 to e18e633f49506a264e427025775423891d85fcbd
Branch pushed to git repo; I updated commit sha1. New commits:
e18e633  Correct answers when 0 is in alpha

comment:64 Changed 2 years ago by
 Commit changed from e18e633f49506a264e427025775423891d85fcbd to baf8ae905722d35a513a971e757b28c75e2b26da
Branch pushed to git repo; I updated commit sha1. New commits:
baf8ae9  Modify one doctest to distinguish t from 1/t

comment:65 Changed 2 years ago by
 Cc jvoight added
The formulas transcribed by Watkins apparently presume that 0 does not occur in alpha
. Since alpha
and beta
must be disjoint, this case is most easily handled as Magma does it, by swapping alpha
with beta
and t
with 1/t
.
This resolves the issue I raised at comment 62, so now I am back to wondering whether this is ready for review.
comment:66 Changed 2 years ago by
 Cc tornaria added
comment:67 Changed 2 years ago by
There are some errors in the documentation that have already been fixed in #23780.
Can you rebase this on top of it?
comment:68 Changed 2 years ago by
 Commit changed from baf8ae905722d35a513a971e757b28c75e2b26da to 65db2d1e386c734afe6e7a9a580b7854e260d4fe
Branch pushed to git repo; I updated commit sha1. New commits:
6cc03a1  Correct docstring formatting, handling of r=0 case

ef1626f  Merge branch 'develop' into t/23780/modify_p_adic_gauss_sum_to_allow_working_directly_in_q_p

45da482  trac 23780 some doc formatting details

65db2d1  Merge branch 't/23780/modify_p_adic_gauss_sum_to_allow_working_directly_in_q_p' into t/23671/23671

comment:69 Changed 2 years ago by
 Branch changed from u/kedlaya/23671 to u/chapoton/23671
 Commit changed from 65db2d1e386c734afe6e7a9a580b7854e260d4fe to 9dde54a6b42f859c35cbd9945c1da426cef5146a
 Dependencies #23585, #23780 deleted
I have made a cosmetic cleanup of the code, to conform to the pep8 standard.
Maybe we whould still add more documentation everywhere ?
New commits:
9dde54a  trac 23671 pep8 cleanup

comment:70 Changed 2 years ago by
 Commit changed from 9dde54a6b42f859c35cbd9945c1da426cef5146a to 28e95f4586177ff08cc06e58dd69c4d5ce17c4df
comment:71 Changed 2 years ago by
 Commit changed from 28e95f4586177ff08cc06e58dd69c4d5ce17c4df to df64dbef1f40c631e6b7a75a481708c0e14d0589
Branch pushed to git repo; I updated commit sha1. New commits:
df64dbe  trac 23671 more care for references

comment:72 Changed 2 years ago by
 Branch changed from u/chapoton/23671 to u/kedlaya/23671
comment:73 Changed 2 years ago by
 Commit changed from df64dbef1f40c631e6b7a75a481708c0e14d0589 to cc53131b6ff960faee21aafdfe35175facf2ce10
comment:74 Changed 2 years ago by
 Branch changed from u/kedlaya/23671 to u/chapoton/23671
 Commit changed from cc53131b6ff960faee21aafdfe35175facf2ce10 to 3cdb07c4ab1b9d5274d68d224209ce0a180a1b64
I fixed the wrong syntax in the new raise statement (it was not py3 compatible).
New commits:
3cdb07c  trac 23671 fix wrong syntax for raise

comment:75 Changed 2 years ago by
Are we ready for review at this point?
comment:76 Changed 2 years ago by
I have some small concerns :
1) we should move the references to the masterreferencefile,
2) I would prefer to have more explanations in the doc everywhere.
But I think it can be set to "needs_review" once 1) is done.
I am going to do 1) right now.
comment:77 Changed 2 years ago by
 Commit changed from 3cdb07c4ab1b9d5274d68d224209ce0a180a1b64 to 1d1039f8b082cb4e36d7ef0e34f6860a5385b9eb
comment:78 Changed 2 years ago by
 Keywords sd91 added
comment:79 Changed 2 years ago by
 Milestone changed from sage8.1 to sage8.2
 Status changed from needs_info to needs_review
While more documentation would be nice, there is overall a big lack of documentation in this whole area of mathematics; anyway I would propose to get some reviewer feedback on that front.
I don't see any causal doctest failures in the patchbot logs.
comment:80 Changed 2 years ago by
 Commit changed from 1d1039f8b082cb4e36d7ef0e34f6860a5385b9eb to 12aa8f264e4b454e991c75d48373a3d6f2f8b0cd
Branch pushed to git repo; I updated commit sha1. New commits:
12aa8f2  trac 23671 better ref to slides

comment:81 Changed 2 years ago by
 Branch changed from u/chapoton/23671 to u/roed/23671
comment:82 Changed 2 years ago by
 Commit changed from 12aa8f264e4b454e991c75d48373a3d6f2f8b0cd to de3f9cbac730e7031a4cc53aa291415660a2c841
 Reviewers set to David Roe
Positive review if you're happy with my reviewer patch.
New commits:
de3f9cb  Reviewer changes to 23671

comment:83 Changed 2 years ago by
It seems that you introduced a TAB somewhere.
Otherwise, I agree with the changes.
comment:84 Changed 2 years ago by
 Branch changed from u/roed/23671 to u/chapoton/23671
 Commit changed from de3f9cbac730e7031a4cc53aa291415660a2c841 to 6bbe53ffd07df48f4a51f6e08c62ff5cfcb7c25e
comment:85 Changed 2 years ago by
 Status changed from needs_review to positive_review
Oops. I guess my emacs doesn't kill tabs in .rst
files.
comment:86 Changed 2 years ago by
 Branch changed from u/chapoton/23671 to u/roed/23671
comment:87 Changed 2 years ago by
 Commit changed from 6bbe53ffd07df48f4a51f6e08c62ff5cfcb7c25e to 0c54a659e37c82c1b98d9299d3dc04dc86aedec0
roed just added a @cached_method
decorator to padic_H_value
, H_value
, and euler_factor
.
I just went over roed's changes (most of which he made while sitting next to me) and they all look fine to me.
New commits:
0c54a65  Add cached_method to hypergeometric motives

comment:88 Changed 2 years ago by
You reintroduced the TAB, please take care of it yourself
comment:89 Changed 2 years ago by
 Status changed from positive_review to needs_work
comment:90 Changed 2 years ago by
 Branch changed from u/roed/23671 to u/chapoton/23671
 Commit changed from 0c54a659e37c82c1b98d9299d3dc04dc86aedec0 to 6bbe53ffd07df48f4a51f6e08c62ff5cfcb7c25e
 Status changed from needs_work to positive_review
comment:91 Changed 2 years ago by
 Commit changed from 6bbe53ffd07df48f4a51f6e08c62ff5cfcb7c25e to 778b876a2895a80c1622c11cb28441211b907ed8
 Status changed from positive_review to needs_review
comment:92 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:93 Changed 2 years ago by
 Status changed from positive_review to needs_work
Merge conflict...
comment:94 Changed 2 years ago by
 Commit changed from 778b876a2895a80c1622c11cb28441211b907ed8 to fd375d34156ef99ead97062523cd6db965d25f88
Branch pushed to git repo; I updated commit sha1. New commits:
fd375d3  Merge branch 'u/chapoton/23671' in 8.1.b7

comment:95 Changed 2 years ago by
 Status changed from needs_work to positive_review
trivial rebase done (damn main reference file, I have not ordered that)
comment:96 Changed 2 years ago by
 Status changed from positive_review to needs_work
PDF docs don't build
comment:97 Changed 2 years ago by
 Commit changed from fd375d34156ef99ead97062523cd6db965d25f88 to d8c961bfe6d67c265a5c615557ad7744cdf42e20
Branch pushed to git repo; I updated commit sha1. New commits:
d8c961b  trac 23671 care for pdf doc

comment:98 Changed 2 years ago by
Is there more to be done on the pdf docs, or should this be set back to positive review?
comment:99 Changed 2 years ago by
Well, somebody needs to check that it works, and I have not got time yet to do that.
comment:101 Changed 2 years ago by
 Milestone changed from sage8.2 to sage8.1
comment:102 Changed 2 years ago by
 Branch changed from u/chapoton/23671 to d8c961bfe6d67c265a5c615557ad7744cdf42e20
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
first sketch of implementation of hypergeometric motives