Opened 20 months ago

Closed 4 months ago

Last modified 5 weeks ago

#29222 closed enhancement (fixed)

Improvements to padic polylog

Reported by: alexjbest Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description

Two improvements for p-adic 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 p-adic logarithm that is used, in the same way as for logarithm.

Change History (27)

comment:1 Changed 18 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:2 Changed 18 months ago by alexjbest

  • Branch set to u/alexjbest/lip

comment:3 Changed 18 months ago by alexjbest

  • Commit set to 0d2b3800a4697ae286f9389f36978604d54a1502
  • Status changed from new to needs_review

New commits:

0d2b380fixes for polylogs

comment:4 Changed 17 months ago by chapoton

  • Status changed from needs_review to needs_work

one failing doctest, see bot report

comment:5 Changed 17 months ago by chapoton

  • Summary changed from Improvements to padic poylog to Improvements to padic polylog

comment:6 Changed 17 months ago by git

  • Commit changed from 0d2b3800a4697ae286f9389f36978604d54a1502 to e983616bfeca554868a277ff9bf99dca2853b002

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

e983616fix doctest

comment:7 Changed 17 months ago by alexjbest

Ooops how did I miss that, thanks for the heads up.

comment:8 Changed 16 months ago by chapoton

  • 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 git

  • Commit changed from e983616bfeca554868a277ff9bf99dca2853b002 to f092ee0fb045282e2d5676576485dc95310d28dc

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

f092ee0define polylogs at 1 using duplication formula

comment:10 Changed 16 months ago by alexjbest

  • Work issues set to wait for patchbots

comment:11 Changed 16 months ago by chapoton

The two new raise ValueError should be doctested.

comment:12 Changed 16 months ago by git

  • Commit changed from f092ee0fb045282e2d5676576485dc95310d28dc to 099e1faa8421c0e23fecf978397165ce89699926

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

099e1faadd test for new valueerror

comment:13 Changed 16 months ago by git

  • Commit changed from 099e1faa8421c0e23fecf978397165ce89699926 to c699a80a48352a18f856d0f8543c12dc6010385f

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

c699a80other valueerror

comment:14 Changed 16 months ago by chapoton

one doctest for the ValueError? does not pass, and seems to trigger the other one instead

comment:15 Changed 15 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:16 Changed 13 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:17 Changed 11 months ago by git

  • Commit changed from c699a80a48352a18f856d0f8543c12dc6010385f to e0abdc0f7fe0aff9c7677430fcd1518ee55fe8de

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

c046b39Merge branch 'master' of git://github.com/sagemath/sage into lip
17099abMerge branch 'develop' of git://github.com/sagemath/sage into lip
e0abdc0use numerical comparisons for logs, fix doctests

comment:18 Changed 11 months ago by alexjbest

  • 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 git

  • Commit changed from e0abdc0f7fe0aff9c7677430fcd1518ee55fe8de to 528197349e2f6aee483ed2ebb5d18cc28b764ca7

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

5281973Merge branch 'develop' of git://github.com/sagemath/sage into lip

comment:20 Changed 11 months ago by alexjbest

The memory leaks could be the same issue as #27536 it appears there are many sage.rings.real_mpfr.RealNumber left on the heap.

Last edited 5 months ago by slelievre (previous) (diff)

comment:21 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:22 Changed 5 months ago by roed

  • 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 alexjbest

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(gsl-1).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 roed

  • Status changed from needs_info to positive_review
  • Work issues fix leaks? deleted

Sounds good to me.

comment:25 Changed 4 months ago by slelievre

Leak issue now tracked at #31826.

comment:26 Changed 4 months ago by vbraun

  • 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 mkoeppe

  • Commit 528197349e2f6aee483ed2ebb5d18cc28b764ca7 deleted
  • Reviewers changed from Frederic Chapoton, David Roe to Frédéric Chapoton, David Roe
Note: See TracTickets for help on using tickets.