Opened 8 years ago

Last modified 22 months ago

#13110 needs_work defect

Deprecate global RealNumber() and ComplexNumber()

Reported by: eviatarbach Owned by: jason, jkantor
Priority: major Milestone: sage-8.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 vdelecroix)

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.

Attachments (1)

trac_13110-python_complex_input-ts.patch (4.6 KB) - added by tscrim 7 years ago.

Download all attachments as: .zip

Change History (51)

comment:1 Changed 7 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Keywords complex beginner added
  • Status changed from new to needs_review

Simple fix and easy to review.

comment:2 Changed 7 years ago by nbruin

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 7 years ago by eviatarbach

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

comment:4 Changed 7 years ago by tscrim

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 7 years ago by chapoton

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 7 years ago by tscrim

comment:6 Changed 7 years ago by tscrim

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

comment:7 follow-up: Changed 7 years ago by 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?

comment:8 in reply to: ↑ 7 Changed 7 years ago by tscrim

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 7 years ago by eviatarbach

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

comment:10 Changed 7 years ago by tscrim

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 7 years ago by nbruin

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 7 years ago by vbraun

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 follow-up: Changed 7 years ago by 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.

  • 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 7 years ago by tscrim

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.

comment:15 follow-up: Changed 7 years ago by 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.

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 7 years ago by eviatarbach

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 7 years ago by nbruin

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.

comment:18 Changed 7 years ago by kcrisman

  • Status changed from needs_review to needs_work

comment:19 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:20 Changed 6 years ago by eviatarbach

For reference, Python's built-in 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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:22 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:23 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:24 Changed 5 years ago by jdemeyer

  • 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 jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:27 Changed 3 years ago by souravsingh

I am interested on working on the issue. How do I start?

comment:28 Changed 3 years ago by zonova

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 3 years ago by epilys

  • Branch set to u/epilys/deprecate_global_realnumber___and_complexnumber__

comment:30 follow-ups: Changed 3 years ago by epilys

  • 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:

c1995f1Allow type complex input in ComplexNumber()

comment:31 Changed 3 years ago by epilys

  • Status changed from needs_work to needs_review

comment:32 in reply to: ↑ 30 Changed 3 years ago by jdemeyer

  • 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 ; follow-up: Changed 3 years ago by zonova

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 inside create_ComplexNumber?


New commits:

c1995f1Allow 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.

Last edited 3 years ago by zonova (previous) (diff)

comment:34 in reply to: ↑ 33 ; follow-up: Changed 3 years ago by 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 like 1.2 + 3.4 * I and that works fine.

comment:35 in reply to: ↑ 34 Changed 2 years ago by vdelecroix

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 like 1.2 + 3.4 * I and that works fine.

That does not

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

comment:36 Changed 2 years ago by vdelecroix

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 2 years ago by vdelecroix

  • Description modified (diff)
  • Keywords complex beginner removed
  • Milestone changed from sage-6.4 to sage-8.2

comment:38 Changed 2 years ago by vdelecroix

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.

comment:39 Changed 23 months ago by vdelecroix

  • Authors changed from Travis Scrimshaw to Vincent Delecroix
  • 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

New commits:

064e66813110: deprecate RealNumber/ComplexNumber
588b9ac13110: fix doctests

comment:40 Changed 23 months ago by vdelecroix

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

comment:41 follow-up: Changed 23 months ago by 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.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 23 months ago by 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.

comment:43 in reply to: ↑ 42 ; follow-up: Changed 23 months ago by vdelecroix

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 23 months ago by git

  • Commit changed from 588b9ac6c7b2b906c0d31f9dd925968d1e300865 to 140f7bdb4665396e88d98d9767c480ee9af990cd

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

140f7bd13110: fix doctests

comment:45 in reply to: ↑ 43 ; follow-up: Changed 23 months ago by 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.

comment:46 in reply to: ↑ 45 ; follow-up: Changed 23 months ago by vdelecroix

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 ; follow-up: Changed 23 months ago by jdemeyer

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 ; follow-up: Changed 23 months ago by vdelecroix

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 23 months ago by vdelecroix

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 _Real

Does 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): 
    476476        Set up Sage command-line environment
    477477        """
    478478        # 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
    480482        initialize_globals(self.all_globals(), self.shell.user_ns)
     483        set_global('_Real', create_RealNumber)
     484        set_global('_Complex', create_ComplexNumber)
    481485        self.run_init()

comment:50 Changed 22 months ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

Note: See TracTickets for help on using tickets.