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

tight complex interval inverse #19964

Closed
miguelmarco opened this issue Jan 26, 2016 · 29 comments
Closed

tight complex interval inverse #19964

miguelmarco opened this issue Jan 26, 2016 · 29 comments

Comments

@miguelmarco
Copy link
Contributor

The current method to compute the multiplicative inverse of a complex interval uses schoolbok formula that produces intervals much bigger than the actual result.

This ticket implements a method that produces a tight interval enclosure of the result.

As a result, we get better precission in root isolating methods (and hence, less steps needed).

Here are some examples that illustrate the impact of the changes:

Old behaviour:

sage: a = CIF((1, 2), (3, 4))
sage: a.real().lower(), a.real().upper()
(1.00000000000000, 2.00000000000000)
sage: a = CIF((1, 2), (3, 4))
sage: b = a.__invert__()
sage: b.real().lower(), b.real().upper()
(0.0499999999999999, 0.200000000000001)
sage: b.imag().lower(), b.imag().upper()
(-0.400000000000001, -0.149999999999999)
sage: b.diameter()
1.20000000000000
sage: %time a.__invert__()
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 23.1 µs
1.? - 1.?*I

new behaviour:

sage: a = CIF((1, 2), (3, 4))
sage: a.real().lower(), a.real().upper()
(1.00000000000000, 2.00000000000000)
sage: b = a.__invert__()
sage: b.real().lower(), b.real().upper()
(0.0588235294117647, 0.153846153846154)
sage: b.imag().lower(), b.imag().upper()
(-0.300000000000001, -0.200000000000000)
sage: b.diameter()
0.893617021276596
sage: %time a.__invert__()
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 19.1 µs
0.1? - 0.3?*I

Another example with a smaller interval,

old:

sage: a = CIF((5, 5.01), (7, 7.01))
sage: b = a.__invert__()
sage: b.diameter()
0.00523867991752868
sage: b.real().lower(), b.real().upper()
(0.0673489564952680, 0.0677027027027028)
sage: b.imag().lower(), b.imag().upper()
(-0.0947297297297298, -0.0942885390933752)
sage: %time a.__invert__()
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 23.1 µs
-0.068? - 0.095?*I

new:

sage: a = CIF((5, 5.01), (7, 7.01))
sage: b = a.__invert__()
sage: b.diameter()
0.00253766599329326
sage: b.real().lower(), b.real().upper()
(0.0674398874563158, 0.0676112447891434)
sage: b.imag().lower(), b.imag().upper()
(-0.0945945945945946, -0.0944232370063658)
sage: a = CIF((-5.01, -5), (7, 7.01))
sage: %time a.__invert__()
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 49.1 µs
-0.068? - 0.0945?*I

As you can see, the diameter of the result can easily get cut to half or even smaller.

The timings of the inversion can vary, but they remain in the same order of magnitude as before. It has a noticeable effect in root isolation:

old:

sage: R.<x> = QQ[]
sage: f = -7/6*x^7 - x^6 + 1/2*x^4 + 2/17*x^3 + 2*x^2 - 6*x - 9
sage: %time f.roots(CC)
CPU times: user 6 ms, sys: 0 ns, total: 6 ms
Wall time: 5.34 ms
[(-1.03157268729039, 1),
 (-1.18964736821330 - 0.850802715570186*I, 1),
 (-1.18964736821330 + 0.850802715570186*I, 1),
 (0.0780792982605128 - 1.39288589462175*I, 1),
 (0.0780792982605128 + 1.39288589462175*I, 1),
 (1.19878298502655 - 0.599304323571307*I, 1),
 (1.19878298502655 + 0.599304323571307*I, 1)]
sage: %time f.roots(QQbar)
CPU times: user 17 ms, sys: 0 ns, total: 17 ms
Wall time: 15.7 ms
[(-1.031572687290387?, 1),
 (-1.189647368213303? - 0.8508027155701860?*I, 1),
 (-1.189647368213303? + 0.8508027155701860?*I, 1),
 (0.07807929826051275? - 1.392885894621755?*I, 1),
 (0.07807929826051275? + 1.392885894621755?*I, 1),
 (1.198782985026555? - 0.5993043235713073?*I, 1),
 (1.198782985026555? + 0.5993043235713073?*I, 1)]

new:

sage: R.<x>=QQ[]
sage: f = -7/6*x^7 - x^6 + 1/2*x^4 + 2/17*x^3 + 2*x^2 - 6*x - 9
sage: %time f.roots(CC)
CPU times: user 4 ms, sys: 0 ns, total: 4 ms
Wall time: 3.28 ms
[(-1.03157268729039, 1),
 (-1.18964736821330 - 0.850802715570186*I, 1),
 (-1.18964736821330 + 0.850802715570186*I, 1),
 (0.0780792982605128 - 1.39288589462175*I, 1),
 (0.0780792982605128 + 1.39288589462175*I, 1),
 (1.19878298502655 - 0.599304323571307*I, 1),
 (1.19878298502655 + 0.599304323571307*I, 1)]
sage: %time f.roots(QQbar)
CPU times: user 9 ms, sys: 1 ms, total: 10 ms
Wall time: 9.3 ms
[(-1.031572687290387?, 1),
 (-1.189647368213303? - 0.8508027155701860?*I, 1),
 (-1.189647368213303? + 0.8508027155701860?*I, 1),
 (0.07807929826051275? - 1.392885894621755?*I, 1),
 (0.07807929826051275? + 1.392885894621755?*I, 1),
 (1.198782985026555? - 0.5993043235713073?*I, 1),
 (1.198782985026555? + 0.5993043235713073?*I, 1)]

CC: @tscrim @videlec @dkrenn @jdemeyer @sagetrac-tmonteil @cheuberg

Component: numerical

Keywords: interval, root

Author: Miguel Marco

Branch: 39558b6

Reviewer: Vincent Delecroix, Marc Mezzarobba

Issue created by migration from https://trac.sagemath.org/ticket/19964

@miguelmarco miguelmarco added this to the sage-7.1 milestone Jan 26, 2016
@miguelmarco
Copy link
Contributor Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 26, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

84ab655compute tightly the inverse of complex interval.
a7a64acadapted division to new inverse
5b7082dfixed doctests
97f4504Merge branch 'inversecomplexintervals' into t/19964/tight_complex_interval_inverse

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 26, 2016

Commit: 97f4504

@miguelmarco
Copy link
Contributor Author

Author: Miguel Marco

@miguelmarco
Copy link
Contributor Author

New commits:

84ab655compute tightly the inverse of complex interval.
a7a64acadapted division to new inverse
5b7082dfixed doctests
97f4504Merge branch 'inversecomplexintervals' into t/19964/tight_complex_interval_inverse

@miguelmarco
Copy link
Contributor Author

Changed keywords from none to interval, root

@miguelmarco

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Jan 26, 2016

comment:5

Can you give an example which clearly illustrates the better bounds? Also, have you run timings versus the schoolbook version? (I don't have the technical abilities to review this ticket though.)

@miguelmarco

This comment has been minimized.

@miguelmarco
Copy link
Contributor Author

comment:6

Added some examples to the description.

@videlec
Copy link
Contributor

videlec commented Mar 9, 2016

comment:7

Is this better in all cases?

-             0.?e-182 - 0.7598602580415435?*I,
-             0.?e-249 + 0.7598602580415435?*I,
+             0.?e-215 - 0.7598602580415435?*I,
+             0.?e-229 + 0.7598602580415435?*I,

@miguelmarco
Copy link
Contributor Author

comment:8

It should be, up to floating point artifacts. I don't know much about the subtleties of floating point numbers though.

@videlec
Copy link
Contributor

videlec commented Mar 9, 2016

comment:9

I was wondering about the e-249 becoming a e-229 in the above example...

@miguelmarco
Copy link
Contributor Author

comment:10

Yes, that is what i imagined, but i would guess that in those two cases the discrepancy is due to those pesky numerical artifacts that can appear just by, for example, performing the same operations in different order.

@mezzarobba
Copy link
Member

comment:12

IMHO the tests should cover all branches of the new implementation (ideally with test cases where the rounding modes matter, if that's not too hard to achieve).

@videlec
Copy link
Contributor

videlec commented Apr 5, 2016

comment:13

Following comment:12 I added a doctest. And I also removed the trailing whitespaces in a second commit.

This is good for me to go. Any remarks?


New commits:

1e5fe26Trac 19964: add doctest for all branches
d57c626Trac 19964: get rid of trailing whitespaces

@videlec
Copy link
Contributor

videlec commented Apr 5, 2016

Reviewer: Vincent Delecroix

@videlec
Copy link
Contributor

videlec commented Apr 5, 2016

Changed commit from 97f4504 to d57c626

@videlec
Copy link
Contributor

videlec commented Apr 5, 2016

Changed branch from u/mmarco/tight_complex_interval_inverse to u/vdelecroix/19964

@videlec videlec modified the milestones: sage-7.1, sage-7.2 Apr 5, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2016

Changed commit from d57c626 to d57fee2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 5, 2016

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

421b3b3Trac 19964: add doctest for all branches
d57fee2Trac 19964: get rid of trailing whitespaces

@mezzarobba
Copy link
Member

comment:15

lgtm except that I'd suggest the following change in order to really test all possible cases:

@@ -1130,12 +1130,15 @@ cdef class ComplexIntervalFieldElement(sage.structure.element.FieldElement):
         Check that the code is valid in all regions of the complex plane::
 
             sage: rats = [15423/906, 337/59976, 145151/145112]
-            sage: for a in rats:
-            ....:     for b in rats:
-            ....:         x = CIF(a, b)
-            ....:         assert (x * (~x) - 1).contains_zero()
-            ....:         x = -CIF(a,b)
-            ....:         assert (x * (~x) - 1).contains_zero()
+            sage: ivs = [RIF(aa, bb) for a in rats for b in rats
+            ....:                   for aa in [-a, a] for bb in [-b, b]
+            ....:                   if aa <= bb]
+            sage: for x in ivs:
+            ....:     for y in ivs:
+            ....:         z = CIF(x, y)
+            ....:         inv = ~z
+            ....:         assert ((z*inv - 1).contains_zero()
+            ....:                 or z.contains_zero() and inv.real().is_NaN())

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 7, 2016

Changed commit from d57fee2 to 39558b6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 7, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

39558b6Trac 19964: extend doctest

@mezzarobba
Copy link
Member

comment:17

You really don't want to test the cases where the interval contains zero ;-). But so be it.

@mezzarobba
Copy link
Member

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Marc Mezzarobba

@vbraun
Copy link
Member

vbraun commented Apr 13, 2016

Changed branch from u/vdelecroix/19964 to 39558b6

@jdemeyer
Copy link

Changed commit from 39558b6 to none

@jdemeyer
Copy link

comment:19

Breakage: #22834

vbraun pushed a commit to vbraun/sage that referenced this issue May 12, 2024
    
The tight reversion of complex intervals introduced in 84ab655 from
sagemath#19964 made complex intervals wrong... and hence unreliable. This
remained unnoticed until an apparently unrelated bug involving QQbar
sagemath#37927. As an emergency measure we simply revert the tight reversion.

Fixes sagemath#37927

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#37941
Reported by: Vincent Delecroix
Reviewer(s): Marc Mezzarobba
unhyperbolic added a commit to unhyperbolic/sage that referenced this issue Jul 13, 2024
…_ - thus for complex interval division. The implementation is inspired by Rokne-Lancaster 1971.

Some history:
- The first implementation of returning tight intervals was commit 84ba655 (see ticket sagemath#19964).
- That implementation had bugs (returning too small intervals) and was reverted to fix ticket sagemath#37941.
- This change is the second implementation.
unhyperbolic added a commit to unhyperbolic/sage that referenced this issue Jul 13, 2024
…_ - thus for complex interval division. The implementation is inspired by Rokne-Lancaster 1971.

Some history:
- The first implementation of returning tight intervals was commit 84ba655 (see ticket sagemath#19964).
- That implementation had bugs (returning too small intervals) and was reverted to fix ticket sagemath#37941.
- This change is the second implementation.
unhyperbolic added a commit to unhyperbolic/sage that referenced this issue Jul 13, 2024
…_ - thus for complex interval division. The implementation is inspired by Rokne-Lancaster 1971.

Some history:
- The first implementation of returning tight intervals was commit 84ba655 (see ticket sagemath#19964).
- That implementation had bugs (returning too small intervals) and was reverted to fix ticket sagemath#37941.
- This change is the second implementation.
vbraun pushed a commit to vbraun/sage that referenced this issue Jul 20, 2024
    
This provides a (hopefully) correct implementation of computing a tight
enclosure for the inverse of a complex interval. It fixes sagemath#37963.

There is some history to this:
Commit 84ab655 from sagemath#19964 introduced a tight enclosure but had some
bugs such as ticket sagemath#37927.
Thus, the that commit (and the subsequent partial fixes) were reverted
in commit 8d59b12, see pull request sagemath#37941.
This is a new implementation of Rokne-Lancaster that I wrote from
scratch.

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

sagemath#37941
    
URL: sagemath#38360
Reported by: Matthias Goerner
Reviewer(s): Marc Culler, Nathan Dunfield
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants