Deprecate global RealNumber() and ComplexNumber()

[eviatarbach]

The RealNumber() and ComplexNumber() global functions are converting string to floating-point numbers. They do not do what their names suggest

sage: RealNumber(1/3)
Traceback (most recent call last):
...
TypeError: unable to convert '1/3' to a real number
sage: ComplexNumber(complex('13.8+6.2j'))
Traceback (most recent call last):
...
TypeError: unable to coerce to a ComplexNumber: <type 'str'>

These two functions are mostly intended for the Sage preparser and shouldn't be used directly. We deprecate them in view of removing them from the global namespace. In the future, we might use the names RealNumber and ComplexNumber as proper factories.

Component: numerical

Author: Vincent Delecroix

Branch/Commit: u/vdelecroix/13110 @ 140f7bd

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

[tscrim]

Changed keywords from none to complex

[tscrim]

Author: Travis Scrimshaw

[tscrim]

comment:1

Simple fix and easy to review.

[nbruin]

comment:2

Don't you want it more along the lines of

    if s_imag is None: 
        if isinstance(s_real, complex): 
            s_imag = s_real.imag 
            s_real = s_real.real 
        else:
            s_imag = 0 

to avoid accepting malformed input such as

ComplexNumber(complex(1,1),1)

perhaps?

[eviatarbach]

comment:3

Also, a typo; 'fixzed'>'fixed'

[tscrim]

comment:4

That is a good idea. Updated (and typo fixzed :P) and now it will also work for CDF and ComplexField elements as well.

[fchapoton]

comment:5

It seems to me that you should explain in the doc that a new kind of input is now allowed. So far, the doc says that the only possible input is a pair of numbers.

[tscrim]

Attachment: trac_13110-python_complex_input-ts.patch.gz

[tscrim]

comment:6

I've updated the documentation and added a (quick 'n' dirty) parser for string inputs as well.

[eviatarbach]

comment:7

This is really nice, but I think the string parser should be more strict in order to enforce a more consistent syntax. How about just accepting the Python complex syntax?

[tscrim]

comment:8

Replying to @eviatarbach:

This is really nice, but I think the string parser should be more strict in order to enforce a more consistent syntax. How about just accepting the Python complex syntax?

I disagree since the output from ComplexNumber(1, 1) is 1.0... + 1.0...*I, so we should allow this as valid input to be consistent with python's complex function and because we do accept python complex strings.

[eviatarbach]

comment:9

This makes sense, but 2.0+5.0I isn't valid syntax in either Python or Sage.

[tscrim]

comment:10

True, but it's the "natural" in between since python complex strings are 2 + 2j and I think a typical user is likely to try 2 + 2i first. However, if you feel strongly about it, I'll remove it since it is easier to add functionality into sage than to remove it; and I'm 95% certain there's a bug in my current implementation that I have to fix (it should fail on 2 + -2*I).

[nbruin]

comment:11

We already have a clash

sage: 2.0j
2.00000000000000*I
sage: implicit_multiplication(True)
sage: 2.0j
NameError: name 'j' is not defined

and I'm pretty sure you won't get to predefine i and j.

[vbraun]

comment:12

I'm a bit scared by the included number parser ;-) Can we use a regex? This would also be better at validating input.

[nbruin]

comment:13

The ticket states that ComplexNumber(complex(...)) doesn't work and proceeds by implementing a more elaborate string parser. That's not the most obvious way of soving the ticket:

• a python complex already has a nice .real() and .imag(), so stuffing them together in a string to just pry them apart is not a smart move. Doing

    if isinstance(s_real, complex):
        s_real, s_imag = s_real.real(), s_real.imag()

makes much more sense to solve the problem at hand.

• The fact that ComplexNumber would assemble a complex number from numerical input by converting to a string is ludicrous in its own right: If you're handed real and imaginary parts in numerical form, you should NOT be converting to strings as an intermediate. That's just asking for precision loss (and horrible performance).

I realize that we have the documentation:

def create_ComplexNumber(s_real, s_imag=None, int pad=0, min_prec=53):
    Return the complex number defined by the strings s_real and
    s_imag as an element of ``ComplexField(prec=n)``,
    where n potentially has slightly more (controlled by pad) bits than
    given by s.

According to that documentation, the proposed input is not valid anyway. By changing that you're changing the interface the routine offers. That's fine, but once you start changing it, you should improve it.

Note that CC(complex("1+2j")) already works. By the looks of it, CC.__call__ is already taking a much saner approach. Perhaps ComplexNumber should share more with that routine? Or be documented that people should really use CC(...)? or perhaps ComplexNumber can be deprecated altogether?

[tscrim]

comment:14

Replying to @nbruin:

The ticket states that ComplexNumber(complex(...)) doesn't work and proceeds by implementing a more elaborate string parser. That's not the most obvious way of soving the ticket:

• a python complex already has a nice .real() and .imag(), so stuffing them together in a string to just pry them apart is not a smart move. Doing

    if isinstance(s_real, complex):
        s_real, s_imag = s_real.real(), s_real.imag()

makes much more sense to solve the problem at hand.

This is already how I've done it done in the current patch.

• The fact that ComplexNumber would assemble a complex number from numerical input by converting to a string is ludicrous in its own right: If you're handed real and imaginary parts in numerical form, you should NOT be converting to strings as an intermediate. That's just asking for precision loss (and horrible performance).

That was the original implementation with two reals as input, and I haven't changed that. Nevertheless, I agree that it should be changed (see below).

I realize that we have the documentation:

def create_ComplexNumber(s_real, s_imag=None, int pad=0, min_prec=53):
    Return the complex number defined by the strings s_real and
    s_imag as an element of ``ComplexField(prec=n)``,
    where n potentially has slightly more (controlled by pad) bits than
    given by s.

According to that documentation, the proposed input is not valid anyway. By changing that you're changing the interface the routine offers. That's fine, but once you start changing it, you should improve it.

Note that CC(complex("1+2j")) already works. By the looks of it, CC.__call__ is already taking a much saner approach. Perhaps ComplexNumber should share more with that routine? Or be documented that people should really use CC(...)? or perhaps ComplexNumber can be deprecated altogether?

How about we check if the input is a (pair of) str, and if not, then it just feeds it to CC in an appropriate fashion? I don't think we should deprecate it because the naive person IMO would likely look for ComplexNumber before CC.

Volker, good point. I don't use regex too often so it's never really something I think of. I'll do that on the next patch.

[vbraun]

comment:15

CC._element_constructor_ is probably slow for string input since it evaluates the input. I'm a bit uncomfortable with that security-wise, though there are certainly bigger fish to fry.

I agree that it would be best if ComplexNumber would just delegate everything to CC. The latter even handles two strings as input. The docstring should reflect that and say that it is preferred to use CC / CDF directly.

[eviatarbach]

comment:16

Hmm. What do you all think of just making ComplexNumber an alias for CC? In fact, I think this could also be done to RealNumber with RR.

[nbruin]

comment:17

Replying to @vbraun:

CC._element_constructor_ is probably slow for string input since it evaluates the input. I'm a bit uncomfortable with that security-wise, though there are certainly bigger fish to fry.

Yes, indeed. That very "eval" instruction is marked with a "TODO", indicating that it could use improvements. I'd say that's the place to a string interpretation routine, rather than in create_ComplexNumber. Note, by the way, that most machinery for making complex numbers sits in sage.rings.complex_number.ComplexNumber.__init__. It can deal with various inputs, including strings for real and imag, by virtue of just calling RR(real) and RR(imag) (where RR is the underlying real field). Thus, the only thing a "complex number" parser would have to do is locate the likely real and imaginary part in the string and pass them on. This could even be put in this routine itself.

There is one thing that create_ComplexNumber does that other routines don't: It takes the string lengths as an indication of the number of bits required in the representation. None of the other routines do that. It also indicates that create_ComplexNumber perhaps shouldn't be used for anything more general, or in its other roles, take a hint from its input to choose what precision to use.

I think in general, guessing precision from number of digits written down is rarely going to produce desirable results, so I'd be for providing the user interface with a default precision (complex) field.

[videlec]

comment:35

Replying to @jdemeyer:

Replying to @sagetrac-zonova:

I think that, for a lot of students, they would easily recognize the command ComplexNumber

Why should they recognize the command ComplexNumber? Most likely they will type something like 1.2 + 3.4 * I and that works fine.

That does not

sage: parent(1.2 + 3.4 * I)
Symbolic Ring

[videlec]

comment:36

I think that the simplest would just be to remove RealNumber and ComplexNumber from the global namespace (ie deprecate them). These functions are intended to parse a very special kind of strings in a very particular way. This can be achieved by other means in the console like RR(my_string)/CC(my_string) as was suggested.

[videlec]

Changed keywords from complex to none

[videlec]

comment:38

If we decide to keep RealNumber/ComplexNumber in the global namespace, I think that what they should be would better be left to #24456 and #24457. This is one more reason to make this ticket used only for deprecation.

[videlec]

Changed author from Travis Scrimshaw to Vincent Delecroix

[videlec]

New commits:

064e668	13110: deprecate RealNumber/ComplexNumber
588b9ac	13110: fix doctests

[videlec]

Changed branch from u/epilys/deprecate_global_realnumber___and_complexnumber__ to u/vdelecroix/13110

[videlec]

Changed commit from c1995f1 to 588b9ac

[videlec]

comment:40

Mostly setting to needs review in order to let patchbots test it!

[jdemeyer]

comment:41

I don't like the verbosity of sage.rings.real_mpfr.create_RealNumber. I would suggest still keeping it in the global namespace but under a (short) private name like _Real.

[videlec]

comment:42

Replying to @jdemeyer:

I don't like the verbosity of sage.rings.real_mpfr.create_RealNumber. I would suggest still keeping it in the global namespace but under a (short) private name like _Real.

Fine for me.

[videlec]

comment:43

Replying to @videlec:

Replying to @jdemeyer:

I don't like the verbosity of sage.rings.real_mpfr.create_RealNumber. I would suggest still keeping it in the global namespace but under a (short) private name like _Real.

Fine for me.

Though a problem is that names starting with underscore in all.py files are not imported.

[sagetrac-git]

Changed commit from 588b9ac to 140f7bd

[sagetrac-git]

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

140f7bd

13110: fix doctests

[jdemeyer]

comment:45

Replying to @videlec:

Though a problem is that names starting with underscore in all.py files are not imported.

Not a big problem, just import _Real and _Complex explicitly.

[videlec]

comment:46

Replying to @jdemeyer:

Replying to @videlec:

Though a problem is that names starting with underscore in all.py files are not imported.

Not a big problem, just import _Real and _Complex explicitly.

From where? This is about the preparser...

[jdemeyer]

comment:47

Replying to @videlec:

From where?

Put this in sage.all:

from sage.rings.real_mpfr import create_RealNumber as _Real

[videlec]

comment:48

Replying to @jdemeyer:

Replying to @videlec:

From where?

Put this in sage.all:

from sage.rings.real_mpfr import create_RealNumber as _Real

Does not work. Names starting with an underscore are ignored by the import * syntax.

[videlec]

comment:49

Replying to @videlec:

Replying to @jdemeyer:

Replying to @videlec:

From where?

Put this in sage.all:

from sage.rings.real_mpfr import create_RealNumber as _Real

Does not work. Names starting with an underscore are ignored by the import * syntax.

This would be a (not very elegant) solution

diff --git a/src/sage/repl/ipython_extension.py b/src/sage/repl/ipython_extension.py
index 90a8049d72..22110fe5ac 100644
--- a/src/sage/repl/ipython_extension.py
+++ b/src/sage/repl/ipython_extension.py
@@ -476,8 +476,12 @@ class SageCustomizations(object):
         Set up Sage command-line environment
         """
         # import outside of cell so we don't get a traceback
-        from sage.repl.user_globals import initialize_globals
+        from sage.repl.user_globals import initialize_globals, set_global
+        from sage.rings.real_mpfr import create_RealNumber
+        from sage.rings.complex_number import create_ComplexNumber
         initialize_globals(self.all_globals(), self.shell.user_ns)
+        set_global('_Real', create_RealNumber)
+        set_global('_Complex', create_ComplexNumber)
         self.run_init()

[fchapoton]

comment:50

does not apply