no method numerical_approx for integers and rationals

[zimmermann6]

sage: a=8
sage: a.n()
8.00000000000000
sage: a.numerical_approx()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)

/home/zimmerma/<ipython console> in <module>()

/usr/local/sage-4.4.2/sage/local/lib/python2.6/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:2628)()

/usr/local/sage-4.4.2/sage/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2828)()

/usr/local/sage-4.4.2/sage/local/lib/python2.6/site-packages/sage/structure/parent.so in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2595)()

AttributeError: 'sage.rings.integer.Integer' object has no attribute 'numerical_approx'

The same holds for a=17/2 for example.

Since n is a shortcut for numerical_approx, it should work with numerical_approx too. In addition, if one uses a variable n is a program, it would be more portable to use numerical_approx.

CC: @kcrisman

Component: basic arithmetic

Author: Gagan Sekhon

Reviewer: Aly Deines, Willem Jan Palenstijn, Joanna Gaski

Merged: sage-4.6.2.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/9032

[cb31b68b-3181-4f46-ab62-081a947f4434]

comment:3

Is there an example when a.numerical_approx() works?

After reading the documentation on numerical_approx(), I don't think it can be used in this manner.

Note that a.N() does not work either for your example.

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:4

a.numerical_approx() needed to be defined in sage.misc.element.pyx. However n() already defined there calls and checks if the element type already has numerical_approx(). So this would cause an infinite loop.

So instead now it calls to see if it has _numerical_approx()

I changed all the files where a specific type defined numerical_approx to _numerical_approx and defined an "empty method" numerical_approx, which just calls _numerical_approx.

So now every type should automatically have a numerical_approx. If a specific one is defined then that is called or it simply coerces it to be a real or complex approx.

[zimmermann6]

Work Issues: malformed patch

[zimmermann6]

comment:5

hi, with Sage 4.6 I got an error while importing the patch:

----------------------------------------------------------------------
| Sage Version 4.6, Release Date: 2010-10-30                         |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
Loading Sage library. Current Mercurial branch is: 9032
sage: hg_sage.import_patch("/tmp/trac_9032.patch")
cd "/usr/local/sage/devel/sage" && hg status
cd "/usr/local/sage/devel/sage" && hg status
cd "/usr/local/sage/devel/sage" && hg import   "/tmp/trac_9032.patch"
applying /tmp/trac_9032.patch
abort: malformed patch a/sage/structure/element.pyx @@ -578,24 +580,26 @@

Paul

[zimmermann6]

comment:6

sorry, I clicked on the wrong button.

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:7

ok another attempt. But this time, I made the changes much simpler, all elements matrix2, expression, heegner which had a numerical_approx, I just added _numerical_approx=numerical_approx.

Added numerical_approx and N =n in elements

changed functional line 1252 so it calls x._numerical_approx. So if we call numerical_approx for any element (for example integers) which does not have it explicitly defined it does not go into an infinite loop

[4dfe6752-6233-4f7f-8e0e-e1363b12c1b7]

comment:8

This needs a commit message.

[34f15714-f0fb-4cdc-a12b-f9764ca5444d]

Changed work issues from malformed patch to none

[34f15714-f0fb-4cdc-a12b-f9764ca5444d]

comment:10

A few minor points:

You can just rename numerical_approx to _numerical_approx in matrix2.pyx without keeping numerical_approx since Matrix is an element so it inherits the default implementation of numerical_approx. Same for expression.pyx.

(On the other hand, heegner.py does need both.)

I would change the docstrings in structure/element.pyx to something shorter than the full output. Maybe the following?

[..., 'is_idempotent', 'is_integral', ...]

Is the (capital) N() for consistency with something, or is that new?

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:11

If I changed matrix2 and expression to not have numerical_approx then the documentation won't show up.

changed element.pyx

And yes N() is for consistency, but I did remove them from expression since it is not needed.

[kcrisman]

Author: Gagan Sekhon

[kcrisman]

Reviewer: Aly Deines, Willem Jan Palenstijn

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:14

This patch won't apply since documentation for misc/functional.py and symbolic/integration/integral.py is changed in rc1. I will update the patch today after I download new version of rc1

[4dfe6752-6233-4f7f-8e0e-e1363b12c1b7]

comment:16

----------------------------------------------------------------------

The following tests failed:

    sage -t  devel/sage/sage/symbolic/function.pyx # 2 doctests failed
    sage -t  devel/sage/sage/misc/randstate.pyx # Time out
    sage -t  devel/sage/sage/interfaces/sage0.py # Time out
    sage -t  devel/sage/sage/interfaces/psage.py # Time out
    sage -t  devel/sage/sage/plot/plot.py # Time out
    sage -t  devel/sage/sage/symbolic/function_factory.py # 1 doctests failed
    sage -t  devel/sage/sage/functions/min_max.py # 3 doctests failed
    sage -t  devel/sage/sage/modular/modform/test.py # 0 doctests failed
    sage -t  devel/sage/sage/misc/sagedoc.py # Time out
    sage -t  devel/sage/sage/schemes/generic/toric_divisor.py # Time out
    sage -t  devel/sage/sage/tests/cmdline.py # Time out
    sage -t  devel/sage/sage/misc/trace.py # 2 doctests failed
    sage -t  devel/sage/sage/parallel/use_fork.py # 0 doctests failed
    sage -t  devel/sage/sage/symbolic/expression.pyx # Time out
    sage -t  devel/sage/sage/geometry/polyhedra.py # Time out
    sage -t  devel/sage/sage/tests/startup.py # 1 doctests failed
----------------------------------------------------------------------

[34f15714-f0fb-4cdc-a12b-f9764ca5444d]

comment:18

mhampton points out there isn't a patch attached.

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:19

Let's try this one more time. I promise this one is not empty. I am currently also running testall on rc1

[4dfe6752-6233-4f7f-8e0e-e1363b12c1b7]

comment:20

Everything finally passes.

[34f15714-f0fb-4cdc-a12b-f9764ca5444d]

comment:21

Confirmed. (Leaving it as positive review after the patch was updated.)

[kcrisman]

comment:22

Replying to @wjp:

Confirmed. (Leaving it as positive review after the patch was updated.)

Except the commit message of the latest patch is somewhat mystifying. O bureaucracy! Maybe a friendly release manager can just update the patch with something like "Ensures everything relevant can be numerically approximated and changes things to an underscore method in a few cases"...

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:23

Added commit message and comments to all changes. Plus added a couple more doctest (just so I don't wake up in the middle of the night afraid something went wrong). I am currently running all doctest to make sure it all still works. And the patch is nonempty, I checked.

For some reason, I can't change to needs_review.

[26c29b88-4f0e-4b03-8ae6-078b44036a6d]

comment:26

All tests pass, and I would positively review this patch, except for three documentation problems.

1) The extraneous, pre-existing example line ("a = 2/3") should be removed (in structure/element.pyx). It is neither an example of the use of numerical approximation, nor used in the remaining part of the example.

sage: a = 8
sage: a.numerical_approx?
...

       Return a numerical approximation of x with at least prec bits of
       precision.

       EXAMPLES:

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

2) Grammatical error in symbolic/expression.pyx:

sage: a = cos(3)
sage: a.numerical_approx?
...

       Return a numerical approximation this symbolic expression as either

Should be "numerical approximation of this symbolic expression...." ^^

I know these are trivial and not written by you. However, they're too trivial for their own patches, and would fit logically in yours.

3) When I create a symbolic expression, I now get different docstrings for "n" (from element) and "numerical_approx" (from expression):

sage: a = cos(3)
sage: type(a)
<type 'sage.symbolic.expression.Expression'>
sage: a.n?
...
       Return a numerical approximation of x with at least prec bits of
       precision.

       EXAMPLES:

          sage: (2/3).n()
          0.666666666666667
...

sage: a.numerical_approx?
...    
       Return a numerical approximation this symbolic expression as either
       a real or complex number with at least the requested number of bits
       or digits of precision.

       EXAMPLES:

          sage: sin(x).subs(x=5).n()
          -0.958924274663138
...

I believe that this happened when you removed the re-definition of method "n" in expression. However, that is the docstring that you want to be returned for a symbolic expression.

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

Attachment: trac_9032.patch.gz

[4c1287a7-ef03-4280-b43d-41d57e0137f9]

comment:27

I made the changes jgaski suggested and ran all test. The patch passed.

[kcrisman]

Changed reviewer from Aly Deines, Willem Jan Palenstijn to Aly Deines, Willem Jan Palenstijn, Joanna Gaski

[kcrisman]

comment:29

I think this is the full name of the last reviewer which I put in the reviewer section, but please advise if it's not correct.

[jdemeyer]

Merged: sage-4.6.2.alpha2

[nthiery]

comment:31

Hi!

Is the alias N really needed? It pollutes the namespace of all elements, and broke some code on the Sage-Combinat queue (in that particular case, it was overwriting a field of a gap3 record element). Well, I guess we can find a workaround but still having n and N and numerical_approx seems like a waste.

Cheers, Nicolas

[kcrisman]

comment:32

Is the alias N really needed? It pollutes the namespace of all elements, and broke some code on the Sage-Combinat queue (in that particular case, it was overwriting a field of a gap3 record element). Well, I guess we can find a workaround but still having n and N and numerical_approx seems like a waste.

N has been an alias for this for a long time, so this just making it consistent - I think this added a method, not a global function. In a Sage version nearly a year old:

sage: N(5)
5.00000000000000
sage: n(5)
5.00000000000000
sage: numerical_approx(5)
5.00000000000000
sage: a=5
sage: a.N?
Object `a.N` not found.
sage: a.n
<built-in method n of sage.rings.integer.Integer object at 0x1091154e0>

[nthiery]

comment:33

Hi!

Replying to @kcrisman:

N has been an alias for this for a long time, so this just making it consistent - I think this added a method, not a global function.

Yes; I am precisely arguing about this method, and the pollution of the method namespace of all elements. We can't use anymore x.N to access an attribute called N in a gap record x (well, we can, but as x.getattr("N") which is not very convenient).

Thanks,

[kcrisman]

comment:34

I see. Well, it's been merged, so the proper thing to do is to ask on sage-devel or open a ticket. I don't see why it wouldn't be possible to take this behavior away from structure/element and put it only on numerical types, though it would be a little more of a pain.

I could imagine someone wanting numerical approx of e.g. GAP elements, though, and it's true that for consistency n and N should do the same thing as methods as as functions, so maybe this is worth raising on sage-devel. I wouldn't be surprised if no one much cared, though, as long as integers and rationals still had N and n.

[nthiery]

comment:35

Replying to @kcrisman:

I see. Well, it's been merged, so the proper thing to do is to ask on sage-devel or open a ticket. I don't see why it wouldn't be possible to take this behavior away from structure/element and put it only on numerical types, though it would be a little more of a pain.

I could imagine someone wanting numerical approx of e.g. GAP elements, though, and it's true that for consistency n and N should do the same thing as methods as as functions, so maybe this is worth raising on sage-devel. I wouldn't be surprised if no one much cared, though, as long as integers and rationals still had N and n.

Do you mind running this discussion on sage-devel?

I guess someone wanting a numerical approximation on a gap element (that could well be me :-)) can afford to use g.numerical_approx(), or just g.n().

Thanks!