#29222 closed enhancement (fixed)
Improvements to padic polylog
Reported by:  alexjbest  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  padics  Keywords:  polylog, padic 
Cc:  caruso  Merged in:  
Authors:  Alex J. Best  Reviewers:  Frédéric Chapoton, David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  5281973 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
Two improvements for padic polylogarithms:
Python2>3 broke range of a rational in a way that didn't trigger any doctest failures, fix it and add a doctest.
Allow the user to specify the branch of the padic logarithm that is used, in the same way as for logarithm.
Change History (27)
comment:1 Changed 18 months ago by
 Milestone changed from sage9.1 to sage9.2
comment:2 Changed 18 months ago by
 Branch set to u/alexjbest/lip
comment:3 Changed 18 months ago by
 Commit set to 0d2b3800a4697ae286f9389f36978604d54a1502
 Status changed from new to needs_review
comment:4 Changed 17 months ago by
 Status changed from needs_review to needs_work
one failing doctest, see bot report
comment:5 Changed 17 months ago by
 Summary changed from Improvements to padic poylog to Improvements to padic polylog
comment:6 Changed 17 months ago by
 Commit changed from 0d2b3800a4697ae286f9389f36978604d54a1502 to e983616bfeca554868a277ff9bf99dca2853b002
Branch pushed to git repo; I updated commit sha1. New commits:
e983616  fix doctest

comment:7 Changed 17 months ago by
Ooops how did I miss that, thanks for the heads up.
comment:8 Changed 16 months ago by
 Cc caruso added
 Status changed from needs_work to needs_review
rather ask an expert of the field
comment:9 Changed 16 months ago by
 Commit changed from e983616bfeca554868a277ff9bf99dca2853b002 to f092ee0fb045282e2d5676576485dc95310d28dc
Branch pushed to git repo; I updated commit sha1. New commits:
f092ee0  define polylogs at 1 using duplication formula

comment:10 Changed 16 months ago by
 Work issues set to wait for patchbots
comment:11 Changed 16 months ago by
The two new raise ValueError
should be doctested.
comment:12 Changed 16 months ago by
 Commit changed from f092ee0fb045282e2d5676576485dc95310d28dc to 099e1faa8421c0e23fecf978397165ce89699926
Branch pushed to git repo; I updated commit sha1. New commits:
099e1fa  add test for new valueerror

comment:13 Changed 16 months ago by
 Commit changed from 099e1faa8421c0e23fecf978397165ce89699926 to c699a80a48352a18f856d0f8543c12dc6010385f
Branch pushed to git repo; I updated commit sha1. New commits:
c699a80  other valueerror

comment:14 Changed 16 months ago by
one doctest for the ValueError? does not pass, and seems to trigger the other one instead
comment:15 Changed 15 months ago by
 Status changed from needs_review to needs_work
comment:16 Changed 13 months ago by
 Milestone changed from sage9.2 to sage9.3
comment:17 Changed 11 months ago by
 Commit changed from c699a80a48352a18f856d0f8543c12dc6010385f to e0abdc0f7fe0aff9c7677430fcd1518ee55fe8de
comment:18 Changed 11 months ago by
 Status changed from needs_work to needs_review
 Work issues changed from wait for patchbots to fix leaks?
There appears to be some memory leak type problems with the polylog functions as the code
for p in prime_range(100): K = Qp(p,prec=10) print(p) for t in K.teichmuller_system(): if t == 1: continue l = t.polylog(2) print(p,l)
eats more and more memory which isn't freed at the end.
I don't think these problems were necessarily introduced in this ticket, but nevertheless I'd like to diagnose them if anyone has any suggestions how to find the leaks or advice about likely culprits in the code.
comment:19 Changed 11 months ago by
 Commit changed from e0abdc0f7fe0aff9c7677430fcd1518ee55fe8de to 528197349e2f6aee483ed2ebb5d18cc28b764ca7
Branch pushed to git repo; I updated commit sha1. New commits:
5281973  Merge branch 'develop' of git://github.com/sagemath/sage into lip

comment:20 Changed 11 months ago by
The memory leaks could be the same issue as #27536 it appears there are many sage.rings.real_mpfr.RealNumber
left on the heap.
comment:21 Changed 7 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:22 Changed 5 months ago by
 Reviewers set to Frederic Chapoton, David Roe
 Status changed from needs_review to needs_info
Overall this looks good to me.
Have you made any progress on the memory leak? I don't have any suggestions off the top of my head. I think it's fine to create another ticket for that issue.
You use the .n()
numerical approximation extensively. Would it be better to specify the precision of the real field to use? I'm also not sure if people like seeing the symbolic expression in some cases. Maybe have a precision parameter that defaults to 53, but if it's set to None uses symbolic logs?
I'm happy giving this a positive review if you don't want to work on either of these.
comment:23 Changed 5 months ago by
I haven't made progress on the leak, #27536 seems to have stalled too, so I think your suggestion of merging this as is and making a separate ticket sounds good.
Most of the .n()
parts are just for determining various cutoffs for correct precision bounds, for most of these numbers only the integer part of the result matters.
I doubt anyone would want to look at them symbolically, or that more than 53 digits of precision would be needed. The motivation for the numerical approximations was that a lot of time was being wasted comparing various symbolic expressions to see which is larger, when comparing numerical approximations was far faster.
I could write these sorts of things as RR(gsl1).log(p)
instead I suppose, but I don't think it would make too much difference?
So I don't propose making further changes, unless you have more thoughts on these two issues mentioned of course!
comment:24 Changed 5 months ago by
 Status changed from needs_info to positive_review
 Work issues fix leaks? deleted
Sounds good to me.
comment:25 Changed 4 months ago by
Leak issue now tracked at #31826.
comment:26 Changed 4 months ago by
 Branch changed from u/alexjbest/lip to 528197349e2f6aee483ed2ebb5d18cc28b764ca7
 Resolution set to fixed
 Status changed from positive_review to closed
comment:27 Changed 5 weeks ago by
 Commit 528197349e2f6aee483ed2ebb5d18cc28b764ca7 deleted
 Reviewers changed from Frederic Chapoton, David Roe to Frédéric Chapoton, David Roe
New commits:
fixes for polylogs