Ticket #4151 (closed enhancement: fixed)

Opened 5 years ago

Last modified 4 years ago

[with patch, with positive review] implementation of Dickman's function

Reported by: robertwb Owned by: was
Priority: major Milestone: sage-3.1.3
Component: number theory Keywords:
Cc: Work issues:
Report Upstream: Reviewers: David Loeffler
Authors: Robert Bradshaw Merged in: 3.1.3.alpha1
Dependencies: Stopgaps:

Attachments

4151-dickmanrho.patch Download (31.4 KB) - added by robertwb 5 years ago.
4151-2.patch Download (5.9 KB) - added by davidloeffler 5 years ago.
4151-dickmanrho3.patch Download (6.6 KB) - added by robertwb 5 years ago.

Change History

Changed 5 years ago by robertwb

comment:1 Changed 5 years ago by davidloeffler

I tried installing this (under Sage 3.1.1) and now I get an error whenever I start up Sage:

/home/david/sage-3.1.1/local/lib/python2.5/site-packages/sage/functions/transcendental.py in <module>()
    378
    379
--> 380 from sage.rings.polynomial.polynomial_real_mpfr_dense import PolynomialRealDense
    381
    382 class DickmanRhoComputer:

ImportError: /home/david/sage-3.1.1/local/lib/python2.5/site-packages/sage/rings/polynomial/polynomial_real_mpfr_dense.so: undefined symbol: mpfr_set_z

I tried doing "sage -ba" and that didn't help. Does it only work if installed against one of the 3.1.2 alpha builds?

David

comment:2 Changed 5 years ago by mabshoff

I am pretty sure this is an issue with setup.py and OSX doing lazy symbols lookup at runtime:

   Extension('sage.rings.polynomial.polynomial_real_mpfr_dense', 
   sources = ['sage/rings/polynomial/polynomial_real_mpfr_dense.pyx']), \ 

Change that to

   Extension('sage.rings.polynomial.polynomial_real_mpfr_dense', 
   sources = ['sage/rings/polynomial/polynomial_real_mpfr_dense.pyx'], 
   libraries = ['mpfr', 'gmp']), \

Cheers,

Michael

Changed 5 years ago by davidloeffler

comment:3 follow-up: ↓ 4 Changed 5 years ago by davidloeffler

  • Summary changed from [with patch, needs review] implementation of Dickman's function to [with patch, with partial review] implementation of Dickman's function

OK, I changed setup.py and ran sage -ba and everything worked. I'm puzzled by what you say about the build problems being an OS X issue: I'm running SuSE Linux 10.3.

All doctests pass fine, but there are a couple of funny things:

  • The definition states that rho(0) is 1 by definition, but your implementation gives
    sage: dickman_rho(0)
    0.000000000000000
    

I encountered this while trying to replicate the plot on the Wikipedia page, which fails nastily because log(rho(0)) is undefined.

Also, as discussed for Bessel functions at ticket #4102, it would be nice if the new dickman_rho function derived from calculus.PrimitiveFunction?, as otherwise it doesn't play nicely with compositions etc:

sage: plot( log(dickman_rho(x)), (x, 0.0001, 15) )
# fails
sage: plot( lambda x: log(dickman_rho(x)), (0.001, 15))
# works, but not very intuitive for new users

Reading the code added to functions/transcendental.py, it's elegant and it obviously works, but it seems to throw away information in one case. Suppose I calculate dickman_rho(x) to d digits of precision. Then I want to know dickman_rho(y) to e digits, where y is slightly bigger than x (or, rather, bigger than 1.1x + 10) but e is *much* smaller than d. Then the code empties the cache and goes ahead and recalculates all of the power series from scratch, despite the fact that it already knows the first few terms to more than enough precision. I don't know if the added complication of getting around this problem is worth it; you'd have to remember not just current_prec() but the precision of the calculation of each series term. I guess in practice it's not something that's likely to be a problem.

Also, perhaps the instance variable self.f should be self._f, since it's very much for internal use. On a related note, perhaps it might be better to have a hidden function _compute_power_series, which avoids users having to know about this mysterious extra argument cache_ring, with a corresponding non-hidden function power_series() that takes only 2 arguments rather than 3.

Anyway, I've uploaded a second patch which does the above attribute hiding, and returns 1 rather than 0 for rho(0); with these small changes I'd be more than happy to see this patch included, although I'm not sure my very limited Sage development experience gives me the authority to say this!

I wonder if there are other parts of the existing Sage library that would benefit from using native MPFR polynomials, in place of Sage polynomials over the MPFR real field?

comment:4 in reply to: ↑ 3 Changed 5 years ago by mabshoff

  • Summary changed from [with patch, with partial review] implementation of Dickman's function to [with patch, needs work] implementation of Dickman's function

Replying to davidloeffler:

OK, I changed setup.py and ran sage -ba and everything worked. I'm puzzled by what you say about the build problems being an OS X issue: I'm running SuSE Linux 10.3.

My point is that the extensions works on OSX, but nowhere else, due to the way the OSX linker handles missing symbols. On all platforms but OSX linking in mpfr and its dependency explicitly fixes the problem.

I also changed the subject line to have the ticket get picked up by the standard reports.

Cheers,

Michael

Changed 5 years ago by robertwb

comment:5 Changed 5 years ago by robertwb

  • Summary changed from [with patch, needs work] implementation of Dickman's function to [with patch, needs review] implementation of Dickman's function

Thanks for your comments and improvements, I've attached a follow up patch. Nice catch about the value at 0.

I agree with you that I could be saving some recalculation, but I don't think it's a common enough use case to justify the additional complexity. In fact, I throw away a lot as I go along--for example I don't want to cache 1000-bit 1000-term polynomials just in case one wants dickman_rho(10) to extremely high precision after computing dickman_rho(100).

I've opened #4168 to use native mpfr polynomials elsewhere, figuring it'd involve changes very irrelevant to this ticket.

comment:6 Changed 5 years ago by davidloeffler

  • Summary changed from [with patch, needs review] implementation of Dickman's function to [with patch, with positive review] implementation of Dickman's function

That looks nice; I'm no expert on how the symbolics interface is supposed to work but the dickman_rho function now seems to behave a lot like the exp function, which is presumably a good guide, and you can do things like

sage:((dickman_rho(x) - 0.0001).find_root(0,10)
5.4478836002803135

which is nice.

The only problem I can see is that dickman_rho.approximate(1) returns a ZeroDivisionError?, and dickman_rho.approximate(0.9) returns NaN. But I can't imagine any user being especially upset by this, as the docstring makes it clear that it's to be used for large values only.

I'd be happy to see this merged.

David

comment:7 Changed 5 years ago by mabshoff

  • Status changed from new to closed
  • Resolution set to fixed

Merged in Sage 3.1.3.alpha1

comment:8 Changed 4 years ago by davidloeffler

  • Reviewers set to David Loeffler
  • Merged in set to 3.1.3.alpha1
  • Authors set to Robert Bradshaw
Note: See TracTickets for help on using tickets.