Opened 7 years ago
Last modified 16 months ago
#13110 needs_work defect
Deprecate global RealNumber() and ComplexNumber()
Reported by:  eviatarbach  Owned by:  jason, jkantor 

Priority:  major  Milestone:  sage8.2 
Component:  numerical  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/vdelecroix/13110 (Commits)  Commit:  140f7bdb4665396e88d98d9767c480ee9af990cd 
Dependencies:  Stopgaps: 
Description (last modified by )
The RealNumber()
and ComplexNumber()
global functions are converting string to floatingpoint 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.
Attachments (1)
Change History (51)
comment:1 Changed 6 years ago by
 Keywords complex beginner added
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
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?
comment:3 Changed 6 years ago by
Also, a typo; 'fixzed'>'fixed'
comment:4 Changed 6 years ago by
That is a good idea. Updated (and typo fixzed :P) and now it will also work for CDF
and ComplexField
elements as well.
comment:5 Changed 6 years ago by
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.
Changed 6 years ago by
comment:6 Changed 6 years ago by
I've updated the documentation and added a (quick 'n' dirty) parser for string inputs as well.
comment:7 followup: ↓ 8 Changed 6 years ago by
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?
comment:8 in reply to: ↑ 7 Changed 6 years ago by
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.
comment:9 Changed 6 years ago by
This makes sense, but 2.0+5.0I
isn't valid syntax in either Python or Sage.
comment:10 Changed 6 years ago by
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
).
comment:11 Changed 6 years ago by
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
.
comment:12 Changed 6 years ago by
I'm a bit scared by the included number parser ;) Can we use a regex? This would also be better at validating input.
comment:13 followup: ↓ 14 Changed 6 years ago by
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. Doingif 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?
comment:14 in reply to: ↑ 13 Changed 6 years ago by
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. Doingif 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. PerhapsComplexNumber
should share more with that routine? Or be documented that people should really useCC(...)
? or perhapsComplexNumber
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.
comment:15 followup: ↓ 17 Changed 6 years ago by
CC._element_constructor_
is probably slow for string input since it evaluates the input. I'm a bit uncomfortable with that securitywise, 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.
comment:16 Changed 6 years ago by
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
.
comment:17 in reply to: ↑ 15 Changed 6 years ago by
Replying to vbraun:
CC._element_constructor_
is probably slow for string input since it evaluates the input. I'm a bit uncomfortable with that securitywise, 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.
comment:18 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:19 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:20 Changed 6 years ago by
For reference, Python's builtin function for generating a complex
from a string is defined at http://hg.python.org/cpython/file/c6880edaf6f3/Objects/complexobject.c as complex_subtype_from_string
. We should probably try to emulate that for our parser.
comment:21 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:22 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:23 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:24 Changed 5 years ago by
 Component changed from numerical to coercion
 Description modified (diff)
 Summary changed from ComplexNumber should be able to accept the Python complex type to Allow coercion complex > CC
comment:25 Changed 5 years ago by
 Component changed from coercion to numerical
 Description modified (diff)
 Summary changed from Allow coercion complex > CC to Deprecate global RealNumber() and ComplexNumber()
Sorry, I shouldn't have taken over this ticket, it's about something else.
comment:26 Changed 5 years ago by
 Description modified (diff)
comment:27 Changed 3 years ago by
I am interested on working on the issue. How do I start?
comment:28 Changed 2 years ago by
I believe all that is needed is changing the ComplexNumber function and the RealNumber? function to point to CC and RR instead.
comment:29 Changed 2 years ago by
 Branch set to u/epilys/deprecate_global_realnumber___and_complexnumber__
comment:30 followups: ↓ 32 ↓ 33 Changed 2 years ago by
 Commit set to c1995f12a8529bdb5c5ff25288b481caf01a40c9
I modified create_ComplexNumber
to allow the input as stated in the ticket description as a temporary fix. How should ComplexNumber and RealNumber? point to CC and RR? Just make them aliases or eg call CC with the same input inside create_ComplexNumber
?
New commits:
c1995f1  Allow type complex input in ComplexNumber()

comment:31 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:32 in reply to: ↑ 30 Changed 2 years ago by
 Status changed from needs_review to needs_info
Replying to epilys:
I modified
create_ComplexNumber
to allow the input as stated in the ticket description as a temporary fix.
But why? I don't think there is much reason to allow RealNumber()
and ComplexNumber
, that's why I think they should be deprecated instead.
comment:33 in reply to: ↑ 30 ; followup: ↓ 34 Changed 2 years ago by
Replying to epilys:
I modified
create_ComplexNumber
to allow the input as stated in the ticket description as a temporary fix. How should ComplexNumber and RealNumber? point to CC and RR? Just make them aliases or eg call CC with the same input insidecreate_ComplexNumber
?
New commits:
c1995f1 Allow type complex input in ComplexNumber()
Yes, I was suggesting that ComplexNumber and RealNumber? be aliases for CC and RR. Unless I am missing some functionality that these are supposed to provide that CC and RR don't. I think that, for a lot of students, they would easily recognize the command ComplexNumber, but might not recognize the command CC, so I think it is useful as an alias.
comment:34 in reply to: ↑ 33 ; followup: ↓ 35 Changed 2 years ago by
Replying to 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.
comment:35 in reply to: ↑ 34 Changed 16 months ago by
Replying to jdemeyer:
Replying to 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 like1.2 + 3.4 * I
and that works fine.
That does not
sage: parent(1.2 + 3.4 * I) Symbolic Ring
comment:36 Changed 16 months ago by
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.
comment:37 Changed 16 months ago by
 Description modified (diff)
 Keywords complex beginner removed
 Milestone changed from sage6.4 to sage8.2
comment:38 Changed 16 months ago by
comment:39 Changed 16 months ago by
 Branch changed from u/epilys/deprecate_global_realnumber___and_complexnumber__ to u/vdelecroix/13110
 Commit changed from c1995f12a8529bdb5c5ff25288b481caf01a40c9 to 588b9ac6c7b2b906c0d31f9dd925968d1e300865
 Description modified (diff)
 Status changed from needs_info to needs_review
comment:40 Changed 16 months ago by
Mostly setting to needs review in order to let patchbots test it!
comment:41 followup: ↓ 42 Changed 16 months ago by
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
.
comment:42 in reply to: ↑ 41 ; followup: ↓ 43 Changed 16 months ago by
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.
comment:43 in reply to: ↑ 42 ; followup: ↓ 45 Changed 16 months ago by
Replying to vdelecroix:
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.
comment:44 Changed 16 months ago by
 Commit changed from 588b9ac6c7b2b906c0d31f9dd925968d1e300865 to 140f7bdb4665396e88d98d9767c480ee9af990cd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
140f7bd  13110: fix doctests

comment:45 in reply to: ↑ 43 ; followup: ↓ 46 Changed 16 months ago by
Replying to vdelecroix:
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.
comment:46 in reply to: ↑ 45 ; followup: ↓ 47 Changed 16 months ago by
Replying to jdemeyer:
Replying to vdelecroix:
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...
comment:47 in reply to: ↑ 46 ; followup: ↓ 48 Changed 16 months ago by
Replying to vdelecroix:
From where?
Put this in sage.all
:
from sage.rings.real_mpfr import create_RealNumber as _Real
comment:48 in reply to: ↑ 47 ; followup: ↓ 49 Changed 16 months ago by
Replying to jdemeyer:
Replying to vdelecroix:
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.
comment:49 in reply to: ↑ 48 Changed 16 months ago by
Replying to vdelecroix:
Replying to jdemeyer:
Replying to vdelecroix:
From where?
Put this in
sage.all
:from sage.rings.real_mpfr import create_RealNumber as _RealDoes not work. Names starting with an underscore are ignored by the
import *
syntax.
This would be a (not very elegant) solution

src/sage/repl/ipython_extension.py
diff git a/src/sage/repl/ipython_extension.py b/src/sage/repl/ipython_extension.py index 90a8049d72..22110fe5ac 100644
a b class SageCustomizations(object): 476 476 Set up Sage commandline environment 477 477 """ 478 478 # import outside of cell so we don't get a traceback 479 from sage.repl.user_globals import initialize_globals 479 from sage.repl.user_globals import initialize_globals, set_global 480 from sage.rings.real_mpfr import create_RealNumber 481 from sage.rings.complex_number import create_ComplexNumber 480 482 initialize_globals(self.all_globals(), self.shell.user_ns) 483 set_global('_Real', create_RealNumber) 484 set_global('_Complex', create_ComplexNumber) 481 485 self.run_init()
Simple fix and easy to review.