Opened 10 years ago
Closed 6 years ago
#13055 closed enhancement (fixed)
Refactor numerical_approx()
Reported by:  D.S. McNeil  Owned by:  Burcin Erocal 

Priority:  minor  Milestone:  sage7.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, GitHub, GitLab)  Commit:  fd79227f589f50f190066f7e636356007544b556 
Dependencies:  Stopgaps: 
Description (last modified by )
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 10 years ago by
Description:  modified (diff) 

comment:2 Changed 10 years ago by
Keywords:  sd40.5 added 

comment:3 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:4 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:5 Changed 8 years ago by
Milestone:  sage6.2 → sage6.3 

comment:6 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:7 Changed 8 years ago by
Description:  modified (diff) 

Summary:  loss of precision via SR → Better default precision for .n() method 
comment:8 Changed 8 years ago by
Description:  modified (diff) 

comment:9 Changed 8 years ago by
Type:  defect → enhancement 

comment:10 Changed 8 years ago by
Milestone:  sage6.4 → sageduplicate/invalid/wontfix 

Status:  new → needs_review 
comment:11 followup: 12 Changed 8 years ago by
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 Changed 8 years ago by
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 followup: 14 Changed 8 years ago by
About hold=True
, that even doesn't seem to work with real numbers:
sage: gamma(1.2345678901234567890, hold=True) 0.9097200568693150729
comment:14 followup: 16 Changed 8 years ago by
Milestone:  sageduplicate/invalid/wontfix → sage6.5 

Priority:  major → minor 
Status:  needs_review → needs_work 
Summary:  Better default precision for .n() method → 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 8 years ago by
Component:  symbolics → documentation 

comment:16 Changed 8 years ago by
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 8 years ago by
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 6 years ago by
Keywords:  days74 added 

Milestone:  sage6.5 → sage7.3 
comment:19 Changed 6 years ago by
Authors:  → Jeroen Demeyer 

Description:  modified (diff) 
comment:20 Changed 6 years ago by
Description:  modified (diff) 

comment:21 Changed 6 years ago by
Component:  documentation → basic arithmetic 

Description:  modified (diff) 
Summary:  Document default precision for .n() method → Refactor numerical_approx() 
comment:22 Changed 6 years ago by
Branch:  → u/jdemeyer/document_default_precision_for__n___method 

comment:23 Changed 6 years ago by
Commit:  → fafdb9c02ad5b9d0c7d85fe3cc4468015d08fd68 

Status:  needs_work → needs_review 
New commits:
fafdb9c  Improve numerical_approx

comment:24 followup: 25 Changed 6 years ago by
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 nonElements to Elements before calling it when necessary)?
comment:25 Changed 6 years ago by
Replying to mmezzarobba:
is there a reason why you didn't put the generic implementation directly in
Element
(and put code to convert nonElements to Elements before calling it when necessary)?
Because of the "put code to convert nonElements to Elements before calling it" part. I don't know how to do that in general.
comment:27 Changed 6 years ago by
Commit:  fafdb9c02ad5b9d0c7d85fe3cc4468015d08fd68 → fd79227f589f50f190066f7e636356007544b556 

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

comment:28 Changed 6 years ago by
Reviewers:  → Marc Mezzarobba 

Status:  needs_review → 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 6 years ago by
Branch:  u/jdemeyer/document_default_precision_for__n___method → fd79227f589f50f190066f7e636356007544b556 

Resolution:  → fixed 
Status:  positive_review → closed 
I doubt that this is a bug. How would you guess the default precision for the
.n()
method?