Opened 10 years ago

Last modified 4 years ago

#13110 needs_work defect

Allow coercion complex -> CC — at Version 24

Reported by: eviatarbach Owned by: jason, jkantor
Priority: major Milestone: sage-8.2
Component: numerical Keywords:
Cc: Merged in:
Authors: Travis Scrimshaw Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

This works fine:

sage: CC(complex('13.8+6.2j'))
13.8000000000000 + 6.20000000000000*I
sage: CDF(complex('13.8+6.2j'))
13.8 + 6.2*I

However, it is a conversion while it should be a coercion.

sage: CC.has_coerce_map_from(complex)
False

This is inconsistent with

sage: CC.has_coerce_map_from(float)
True

Change History (25)

comment:1 Changed 9 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 9 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 9 years ago by eviatarbach

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

comment:4 Changed 9 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 9 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 9 years ago by tscrim

comment:6 Changed 9 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 9 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 9 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 9 years ago by eviatarbach

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

comment:10 Changed 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 years ago by kcrisman

  • Status changed from needs_review to needs_work

comment:19 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:20 Changed 8 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 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:22 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:23 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:24 Changed 7 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
Note: See TracTickets for help on using tickets.