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: |
Description (last modified by )
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
- Keywords complex beginner added
- Status changed from new to needs_review
comment:2 Changed 9 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 9 years ago by
Also, a typo; 'fixzed'>'fixed'
comment:4 Changed 9 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 9 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 9 years ago by
comment:6 Changed 9 years ago by
I've updated the documentation and added a (quick 'n' dirty) parser for string inputs as well.
comment:7 follow-up: ↓ 8 Changed 9 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 9 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 9 years ago by
This makes sense, but 2.0+5.0I
isn't valid syntax in either Python or Sage.
comment:10 Changed 9 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 9 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 9 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 follow-up: ↓ 14 Changed 9 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 9 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 follow-up: ↓ 17 Changed 9 years ago by
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
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
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
- Status changed from needs_review to needs_work
comment:19 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:20 Changed 8 years ago by
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
- Milestone changed from sage-6.1 to sage-6.2
comment:22 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:23 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:24 Changed 7 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
Simple fix and easy to review.