Opened 7 years ago

Closed 3 years ago

#13055 closed enhancement (fixed)

Refactor numerical_approx()

Reported by: dsm Owned by: burcin
Priority: minor Milestone: sage-7.3
Component: basic arithmetic Keywords: numerical_approx, sd40.5, days74
Cc: benjaminjones Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: fd79227 (Commits) Commit: fd79227f589f50f190066f7e636356007544b556
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Drop the _numerical_approx method, instead use numerical_approx. Move the generic implementation to a new Cython file. Also put the conversion of decimal digits to bits in one place.

Finally, the documentation of numerical_approx should be made more clear and consistent.

Change History (29)

comment:1 Changed 7 years ago by dsm

  • Description modified (diff)

comment:2 Changed 7 years ago by dsm

  • Keywords sd40.5 added

comment:3 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:5 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:6 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:7 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from loss of precision via SR to Better default precision for .n() method

I doubt that this is a bug. How would you guess the default precision for the .n() method?

comment:8 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 5 years ago by jdemeyer

  • Type changed from defect to enhancement

comment:10 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

comment:11 follow-up: Changed 5 years ago by kcrisman

Hmm, I'm not sure this is wontfix, though. I'm sympathetic to your argument, but Doug's point that

sage: sin(1.2345678901234567890)
0.9440057250452665781
sage: sin(1.2345678901234567890).n()
0.944005725045267

seems good; you shouldn't lose precision, should you? (Forget about hold!)

comment:12 in reply to: ↑ 11 Changed 5 years ago by jdemeyer

Losing precision is a feature of the .n() method, that's what it does. The x.n() method asks for a numerical approximation of x to a precision of 53 bits. This is easily explained. I also like the fact that neither the implementation nor the user should need to worry about what x is, the output is a real/complex number with 53 bits of precision.

If you think that .n() is wrong, what should (2/3).n() return then? The only possible answer which doesn't lose precision is the rational 2/3.

comment:13 follow-up: Changed 5 years ago by jdemeyer

About hold=True, that even doesn't seem to work with real numbers:

sage: gamma(1.2345678901234567890, hold=True)
0.9097200568693150729

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by kcrisman

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-6.5
  • Priority changed from major to minor
  • Status changed from needs_review to needs_work
  • Summary changed from Better default precision for .n() method to Document default precision for .n() method

About hold=True, that even doesn't seem to work with real numbers:

Exactly, which is why I didn't use that example.

Okay, I don't have that much invested in this. But then

sage: x = 1.2340000000000000000000001
sage: x.n?
Docstring:
   Return a numerical approximation of x with at least prec bits of
   precision.

   EXAMPLES:

      sage: (2/3).n()
      0.666666666666667
      sage: pi.n(digits=10)
      3.141592654
      sage: pi.n(prec=20)   # 20 bits
      3.1416

   TESTS:

   Check that http://trac.sagemath.org/14778 is fixed:

      sage: (0).n(algorithm='foo')
      0.000000000000000

should really be improved, and possibly the documentation for other versions of .numerical_approx as well. Those are practically stubs!

comment:15 Changed 5 years ago by rws

  • Component changed from symbolics to documentation

comment:16 in reply to: ↑ 14 Changed 4 years ago by jdemeyer

Replying to kcrisman:

Okay, I don't have that much invested in this. But then x.n? should really be improved, and possibly the documentation for other versions of .numerical_approx as well. Those are practically stubs!

This is actually a general problem we have with Sage documentation: often one has similar functions/methods which all do the same thing (on different kinds of objects). In this case, the numerical_approx function in src/sage/misc/functional.py is much better documented. What's the optimal solution which doesn't simply replicate the documentation from numerical_approx()?

comment:17 Changed 4 years ago by jdemeyer

One other detail which is wrong in the "at least" in "at least prec bits of precision". Certainly for the global numerical_approx() function, it's exactly prec bits.

comment:18 Changed 3 years ago by jdemeyer

  • Keywords days74 added
  • Milestone changed from sage-6.5 to sage-7.3

comment:19 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:20 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:21 Changed 3 years ago by jdemeyer

  • Component changed from documentation to basic arithmetic
  • Description modified (diff)
  • Summary changed from Document default precision for .n() method to Refactor numerical_approx()

comment:22 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/document_default_precision_for__n___method

comment:23 Changed 3 years ago by jdemeyer

  • Commit set to fafdb9c02ad5b9d0c7d85fe3cc4468015d08fd68
  • Status changed from needs_work to needs_review

New commits:

fafdb9cImprove numerical_approx

comment:24 follow-up: Changed 3 years ago by mmezzarobba

Most of the changes look good to me, but I don't like the new description of RealLiteral.numerical_approx(): as far as I understand “real literals” are exact decimal numbers, so converting one to a RealField element with d·log₁₀ 2 bits of precision doesn't really “change its precision” to d decimal digits.

And—just wondering, I have nothing against that choice—is there a reason why you didn't put the generic implementation directly in Element (and put code to convert non-Elements to Elements before calling it when necessary)?

comment:25 in reply to: ↑ 24 Changed 3 years ago by jdemeyer

Replying to mmezzarobba:

is there a reason why you didn't put the generic implementation directly in Element (and put code to convert non-Elements to Elements before calling it when necessary)?

Because of the "put code to convert non-Elements to Elements before calling it" part. I don't know how to do that in general.

comment:26 Changed 3 years ago by jdemeyer

ping

comment:27 Changed 3 years ago by git

  • Commit changed from fafdb9c02ad5b9d0c7d85fe3cc4468015d08fd68 to fd79227f589f50f190066f7e636356007544b556

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fd79227Improve numerical_approx

comment:28 Changed 3 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

I still disagree about the docstring of RealLiteral.numerical_approx(), but let's not again delay the ticket by two months...

comment:29 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/document_default_precision_for__n___method to fd79227f589f50f190066f7e636356007544b556
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.