Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] ComplexFloat class #12192

Open
wants to merge 77 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@cbm755
Copy link
Contributor

cbm755 commented Feb 17, 2017

Eventually this will fix #12154. Here for discussion about whether we want such a class...

TODO list:

  • basic implementation
  • printing
    • exact format of printing TBD.
    • think about 1.2+3.4j some more,
    • code printing
  • proper precision support
    • 15 digits hardcoded in some places
    • need tests for various precisions
    • need tests for mixed precision input for real and imag inputs
    • dps and prec support, with dps default.
  • nan behaviour?
  • infinity behaviour?
  • Float greedily makes ComplexFloat by combining with I etc
    • probably more cases to do
  • fix conjugate().
  • fix a printing bug for ComplexFloat(...)*Symbol('x')
    pprint(S(1+2j)*x) looks like 1.0 + 2.0ⅈ⋅x: it needs paren.
  • ComplexFloat((1.1, 1.2)) should probably work
  • ComplexFloat((<mpf>, <mpf>)) should probably work
  • ComplexFloat(<mpc>) should work
    • tested?
  • S(1+2j) % 2 causes recursion.
  • @asmeuer said: "A real ComplexFloat should perhaps just return a Float." Perhaps so. Maybe ._new should do this but __new__ should not. (S.Zero already has special treatment in ._new.
    • fft/ifft can pickup zero components of ComplexFloat, see discrete/transforms.py doctests
    • argument against doing this: signed zero is useful for branch cuts: ComplexFloat(-1, -0.0) vs ComplexFloat(-1, 0.0).
  • What should Float(complex(1,2)) do? What about Float(ComplexFloat(1,2))? Should both return ComplexFloat? Right now, they are errors. I think this results in the test failures in:
    • geometry/test/test_ellipse.py
    • integrals/test/test_integrals.py
  • sorting of lists of ComplexFloat and Floats is breaking a test in polys/tests/test_polyroots.py.
  • Some failing tests that I'm uncertain about:
    • core/tests/test_complex: 4j*x == 4*x*I
      For sure, 4j*x should become ComplexFloat(0,4)*x is just a question as to whether that should be equal to the 4*x*I. For example, ComplexFloat(0,4) == 4*I is currently True.
    • core/tests/test_function.py: isinstance(sin((1.0 + 1.0*I)**10000 + 1), sin).
    • sympy/core/tests/test_sympify.py: numpy thing, #13275 would fix this.
    • sympy/solvers/tests/test_solvers.py: can reproduce on 1.1.1, filed #13298
    • sympy/integrals/tests/test_meijerint.py: filed #14980, helps here.
    • TODO: some others to list...

Possible future work

  • storage: currently uses two Floats, but could use a mpmath.mpc which is already used for some arithmetic.
  • ComplexFloat("1.23 + 2.34j"): should that work? complex('1+2j') doesn't.
  • Search code for is_Float and see what should be done for ComplexFloat (probably a rather big job)
  • @asmeuer suggested a common subclass for Float and ComplexFloat.

Release notes

  • core
    • new ComplexFloat class improves floating point arithmetic

@aktech aktech added the Enhancement label Feb 23, 2017

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Feb 24, 2017

I've often thought it would be useful to have a complex type. Just doing it for floating point complex numbers seems like a good idea. That means that things like powers and products of non-float complex numbers can still be represented unevaluated.


@property
def _prec(self):
return min(self.real._prec, self.imag._prec)

This comment has been minimized.

@asmeurer

asmeurer Feb 24, 2017

Member

We are pretending that the ComplexFloat has a uniform precision, but storing the component precisions as they were? Would it be better to just do prec = min(real._prec, imag._prec) in the constructor? Actually, as it is now, it doesn't even look like you can have distinct precisions in the constructor.

is_complex = True

def __new__(cls, real, imag=None, prec=None):
# TODO: do something with prec

This comment has been minimized.

@asmeurer

asmeurer Feb 24, 2017

Member

Should also accept a string, Python builtin float/complex types, and tuple (like _mpc_).

__slots__ = ['real', 'imag']

# each Float represents many real numbers,
# both erational and irrational.

This comment has been minimized.

@asmeurer
return self.imag.is_zero

@property
def is_imaginary(self):

This comment has been minimized.

@asmeurer

asmeurer Feb 24, 2017

Member

These should also use the _eval_is_imaginary form.

This comment has been minimized.

@asmeurer

asmeurer Mar 1, 2017

Member

Grep for _eval_is_. The methods define the assumptions (instead of using @property).

def is_real(self):
# TODO: see is_rational above
#return False if self.imag.is_nonzero else None
return self.imag.is_zero

This comment has been minimized.

@asmeurer

asmeurer Feb 24, 2017

Member

A real ComplexFloat should perhaps just return a Float.

@@ -1367,7 +1532,7 @@ def __add__(self, other):
elif isinstance(other, Rational):
#TODO: this can probably be optimized more
return Rational(self.p*other.q + self.q*other.p, self.q*other.q)
elif isinstance(other, Float):
elif isinstance(other, (Float, ComplexFloat)):

This comment has been minimized.

@asmeurer

asmeurer Feb 24, 2017

Member

Should probably have a common superclass.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Feb 24, 2017

Please test that everything works with higher than the default precision. There were too many instances in Float (some still not fixed!) of things assuming the default precision, or falling back to float, and I don't want to repeat the same mistakes.

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Feb 24, 2017

I'm unsure about the printing. Do you think it should print similar to complex, with parentheses no space around the operator, to emphasize that it is a single object? It could be confusing that there's a +, but it's not an Add. OTOH, I'm worried that printing it like that is bad for readability.

@cbm755

This comment has been minimized.

Copy link
Contributor Author

cbm755 commented Feb 24, 2017

I've made some progress on this offline---sorry for not pushing before you read through this :( I will incorporate your suggestions.

re: printing: for now, I've gone back to "0.1 + 0.3*I" as that minimizes the changes needed to existing tests. But I don't like it b/c it looks like an Add and a Mul. I like your idea of no spaces: I'm thinking "0.1+0.3i" (without parenthesis) would be good eventually (or maybe j)

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Feb 24, 2017

j would make it valid Python.

cbm755 added some commits Feb 16, 2017

Add a ComplexFloat class
This will enscapulate `Float(a) + Float(b)*I`, allowing operators on
such.
Mul: greedily combine Float, ComplexFloat and I
After the flattening, we may have a Float/ComplexFloat next to I.
These can be combined into a ComplexFloat.
Add: greedily combine Float, ComplexFloat and I
After the flattening, we may have a Float/ComplexFloat in the head of the
list, followed by a Number*I.  These can make ComplexFloat.
mpmathify: call ComplexFloat in the mpc case
I don't think this changes anything, but perhaps its a little faster
to call it directly?
ComplexFloat: fix in Mul for when "I*Pi"
Maybe there are other cases and we cannot assume the "I" will move to
the front of the list.  But for now, special case for "Pi*I".
ComplexFloat: switch printing to "a + b*I" and update tests
So many tests fail otherwise.  Also, start updating tests that now
need "0.0 + x.y*I" instead of "x.y*I".
ComplexFloat: compare across type
Like current Float behaviour, allow ComplexFloat to equal things that
are not ComplexFloats.
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 26, 2018

Hi, I am the SymPy bot (v124). I'm here to make sure this pull request has a release notes entry. Please read the guide on how to write release notes.

Click here to see the pull request description that was parsed.

Eventually this will fix #12154.  Here for discussion about whether we want such a class...

### TODO list:
- [x] basic implementation
- [x] printing
    - [x] exact format of printing TBD.
    - [x] think about `1.2+3.4j` some more, 
    - [ ] code printing
- [ ] proper precision support
    - [x] ~~15 digits hardcoded in some places~~
    - [x] need tests for various precisions
    - [ ] need tests for mixed precision input for real and imag inputs
    - [x] dps and prec support, with dps default.
- [x] nan behaviour?
- [x] infinity behaviour?
- [x] Float greedily makes ComplexFloat by combining with `I` etc
    - [x] probably more cases to do
- [x] fix `conjugate()`.
- [x] fix a printing bug for ComplexFloat(...)*Symbol('x')
    `pprint(S(1+2j)*x)` looks like `1.0 + 2.0ⅈ⋅x`: it needs paren.
- [x] `ComplexFloat((1.1, 1.2))` should probably work
- [x] `ComplexFloat((<mpf>, <mpf>))` should probably work
- [x] `ComplexFloat(<mpc>)` should work
    - [x] tested?
- [x] `S(1+2j) % 2` causes recursion.
- [ ] @asmeuer said: "A real ComplexFloat should perhaps just return a Float." Perhaps so.  Maybe `._new` should do this but `__new__` should not.  (S.Zero already has special treatment in `._new`.
    - [ ] fft/ifft can pickup zero components of ComplexFloat, see discrete/transforms.py doctests
    - [ ] argument against doing this: signed zero is useful for branch cuts: `ComplexFloat(-1, -0.0)` vs `ComplexFloat(-1, 0.0)`.
- [ ] What should `Float(complex(1,2))` do?  What about `Float(ComplexFloat(1,2))`?  Should both return ComplexFloat?  Right now, they are errors.  I think this results in the test failures in:
   - [ ] geometry/test/test_ellipse.py
   - [ ] integrals/test/test_integrals.py 
- [x] sorting of lists of ComplexFloat and Floats is breaking a test in ` polys/tests/test_polyroots.py`.
- [ ] Some failing tests that I'm uncertain about:
    - [ ] core/tests/test_complex:  `4j*x == 4*x*I`
         For sure, `4j*x` should become `ComplexFloat(0,4)*x` is just a question as to whether that should be equal to the `4*x*I`.  For example, `ComplexFloat(0,4) == 4*I` is currently `True`.
    - [x] core/tests/test_function.py: `isinstance(sin((1.0 + 1.0*I)**10000 + 1), sin)`.
    - [x] sympy/core/tests/test_sympify.py: numpy thing, #13275 would fix this.
    - [x] sympy/solvers/tests/test_solvers.py: can reproduce on 1.1.1, filed #13298
    - [x] sympy/integrals/tests/test_meijerint.py: filed #14980, helps here.
    - [ ] TODO: some others to list...

### Possible future work
- [ ] storage: currently uses two Floats, but could use a `mpmath.mpc` which is already used for some arithmetic.
- [ ] `ComplexFloat("1.23 + 2.34j")`: should that work?  `complex('1+2j')` doesn't.
- [ ] Search code for `is_Float` and see what should be done for `ComplexFloat` (probably a rather big job)
- [ ] @asmeuer suggested a common subclass for Float and ComplexFloat.

#### Release notes

<!-- BEGIN RELEASE NOTES -->
* core
  * new ComplexFloat class improves floating point arithmetic
<!-- END RELEASE NOTES -->

Your release notes are in good order.

Here is what the release notes will look like:

  • core
    • new ComplexFloat class improves floating point arithmetic (#12192 by @cbm755)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.2.1.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

cbm755 added some commits Jul 26, 2018

test: should be fast but its ok if it evaluates
It will now that we have ComplexFloat.
ComplexFloat: use is_Foo instead of isinstance
As per 835cb82, this is faster but
does require that you know you have a sympy object.
ComplexFloat: add TODO comments while we decide what we want
This test specifically states that `S(1.0)` and `S(1.0+0j)` do the same
thing and its not clear we want them too.  On the other hand, looking at
the original report, maybe its enough that both produce something floating
point, not necessarily the same thing...
convolution: fix many accidental floats in tests
I don't think these are supposed to be python floats; many other
nearby quantities use `S` to ensure they are not floats.  So do more
of that.
@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Jul 26, 2018

numerics was an old submodule that was removed a long time ago, which I included in the release notes bot accidentally (sympy/sympy-bot#16). This should be listed under core, because that's where the code is.

cbm755 added some commits Jul 26, 2018

fft: doc that float ifft can add spurious imaginary components
Perhaps if ComplexFloat automatically reduces to Float, this could
be reverted.  (Although its still true that in floating point, imag
components of size machine epsilon can appear so the doc might be
useful anyway.)
Revert "ComplexFloat: use is_Foo instead of isinstance"
This reverts commit 95c9f74.

Woah back, careful don't forget not everything in SymPy inherits from
Basic.  These kinds of improvments should be done carefully and not
as part of some huge PR...
ComplexFloat: use is_Integer because we know its a Number
(and is_Integer is cheaper than isinstance)
Use `isinstance()` instead of `is_Foo` in some cases
For consistency with surrounding code and safety for non-Basic
objects.
ComplexFloat: fix pretty printing Mul
Not sure why this isn't based on precedence, so left a comment in the
code to look at again later.
fix accidental floats in a test
I think these were supposed to be rationals
codegen ast test: minor change for ComplexFloat
A ComplexFloat inherits the minimal precision of its real and imag
parts.  This is intentional.  The setup for this test assumed
otherwise.  Adjust the setup, but I believe the meaning of the test
is unchanged.
factor_terms: don't assume `as_coeff_Mul()[0]` is real
As far as I can tell, this code really wants to know if -1 can be
extracted from each term.  Well, we don't need to check `< 0` to do
that; use `.extract_multiplicative(-1)` instead.  It should still be
cheap because `.as_coeff_Mul()` is supposed to be a cheap operation.

Before:
```
>>> %timeit factor_terms(-x - Catalan*I - 1)
483 µs ± 7.54 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

>>> %timeit factor_terms(-x - Catalan*I - 1)
474 µs ± 2.66 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```

After:
```
>>> %timeit factor_terms(-x - Catalan*I - 1)
492 µs ± 8.87 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

>>> %timeit factor_terms(-x - Catalan*I - 1)
503 µs ± 12.4 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```

cbm755 added some commits Jul 27, 2018

polys: replace accidental floats in tests
These are supposed to test against Rationals.
quantum test: avoid floats in matrix
I don't think this test is really about floats, so create a Rational
Matrix instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.