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

Revert cif fast inversion 84ab655 #37941

Merged
merged 3 commits into from
May 12, 2024

Conversation

videlec
Copy link
Contributor

@videlec videlec commented May 5, 2024

The tight reversion of complex intervals introduced in 84ab655 from #19964 made complex intervals wrong... and hence unreliable. This remained unnoticed until an apparently unrelated bug involving QQbar #37927. As an emergency measure we simply revert the tight reversion.

Fixes #37927

📝 Checklist

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

The aim of commit 84ab655 was to introduced tighter intervals for
inversion. However it turns out that it is wrong in some instances.
@mezzarobba
Copy link
Member

OK, though of course it would be better to debug the inversion routine if somebody has time for that.

@mezzarobba
Copy link
Member

Sorry for the multiple label changes. I agree with the revert, but the tests currently fail, tough I don't think this is due to the changes made here.

Also, you might want to add a test based on #37927 (comment).

@fchapoton
Copy link
Contributor

For a tentative to repair the currently very broken CI , see #37926

@videlec
Copy link
Contributor Author

videlec commented May 6, 2024

Sorry for the multiple label changes. I agree with the revert, but the tests currently fail, tough I don't think this is due to the changes made here.

Also, you might want to add a test based on #37927 (comment).

Isn't that enough?

commit 7a00b56dca6a148cc577aab9b3ffd647d4204c15
Author: Vincent Delecroix <[email protected]>
Date:   Mon May 6 00:05:36 2024 +0200

    add doctests

diff --git a/src/sage/rings/complex_interval.pyx b/src/sage/rings/complex_interval.pyx
index eb8c4f00da..8276d1e31e 100644
--- a/src/sage/rings/complex_interval.pyx
+++ b/src/sage/rings/complex_interval.pyx
@@ -1149,6 +1149,11 @@ cdef class ComplexIntervalFieldElement(FieldElement):
             sage: 1 / CIF(RIF(-1,1),0)
             [.. NaN ..] + [.. NaN ..]*I
 
+        Test that the bug reported in :issue:`37927` is fixed::
+
+            sage: (961 * (1 / CIF(0, 31))**2 + 1).contains_zero()
+            True
+
         REFERENCES:
 
         - [RL1971]_
diff --git a/src/sage/rings/qqbar.py b/src/sage/rings/qqbar.py
index 4e4454988c..773853ec67 100644
--- a/src/sage/rings/qqbar.py
+++ b/src/sage/rings/qqbar.py
@@ -552,6 +552,28 @@ Check that :issue:`28530` is fixed::
      (0.999999587? + 0.?e-11*I, 1),
      (0.999999999? + 0.?e-11*I, 1)]
 
+Check that issue:`37927` is fixed::
+
+    sage: y = polygen(QQ, 'y')
+    sage: v1 = QQbar.polynomial_root(y**2 + 1, CIF(0, -1))
+    sage: v2 = -QQbar(2).sqrt()
+    sage: M = matrix(QQbar, [[0, 0, 1, 0, 0, 0, 0, 0, 0, 0],
+    ....:              [0, 1, 0, 0, 0, 0, 0, 0, 0, 0],
+    ....:              [-4, 2*v1, 1, 64, -32*v1, -16, 8*v1, 4, -2*v1, -1],
+    ....:              [4*v1, 1, 0, -192*v1, -80, 32*v1, 12, -4*v1, -1, 0],
+    ....:               [2, 0, 0, -480, 160*v1, 48, -12*v1, -2, 0, 0],
+    ....:               [-4, 2*I, 1, 64, -32*I, -16, 8*I, 4, -2*I, -1],
+    ....:               [4*I, 1, 0, -192*I, -80, 32*I, 12, -4*I, -1, 0],
+    ....:               [2, 0, 0, -480, 160*I, 48, -12*I, -2, 0, 0],
+    ....:               [0, 0, 0, 8, 4*v2, 4, 2*v2, 2, v2, 1],
+    ....:               [0, 0, 0, 24*v2, 20, 8*v2, 6, 2*v2, 1, 0],
+    ....:               [0, 0, 0, 8, 4*v2, 4, 2*v2, 2, -v2, 1],
+    ....:               [0, 0, 0, 24*v2, 20, 8*v2, 6, 2*v2, 1, 0],
+    ....:               [0, 0, 0, -4096, -1024*I, 256, 64*I, -16, -4*I, 1],
+    ....:               [0, 0, 0, -4096, 1024*I, 256, -64*I, -16, 4*I, 1]])
+    sage: M.right_kernel_matrix()
+    [   1.000000000000000? + 0.?e-16*I                                 0                                 0 -0.00925925925925926? + 0.?e-19*I               0.?e-35 + 0.?e-18*I -0.11111111111111111? + 0.?e-18*I               0.?e-34 + 0.?e-17*I   0.5555555555555555? + 0.?e-16*I                                 0  -0.5925925925925926? + 0.?e-17*I]
+
 AUTHOR:
 
 - Carl Witty (2007-01-27): initial version

@miguelmarco
Copy link
Contributor

As I mentioned in #37927, I think there is another underlying problem with mpfr not being able to find the difference between two different numbers:

sage: c = CIF(0,31)
sage: ci = 1/c
sage: ci.real().endpoints()
(0.000000000000000, -0.000000000000000)
sage: ci.imag().endpoints()
(-0.0322580645161291, -0.0322580645161290)
sage: ci.diameter()
0.000000000000000
sage: ci.imag().endpoints()[1]-ci.imag().endpoints()[0]
0.000000000000000

My guess is that in that particular case, the difference is in the last bit, but one of the numbers uses a higher exponent (both examples I have seen involve the inverses of powers of 2 or powers of two minus one, which are where you would expect to find this kind corner cases), so the precision of the result is set to the bigger exponent, and that last bit of difference is lost.

This problem is exposed in this case because the result of the inversion method is too tight, but I think it would be worth detecting this kind of problems and fixing them, because there are important performance advantages in using tight results.

@videlec
Copy link
Contributor Author

videlec commented May 7, 2024

As I mentioned in #37927, I think there is another underlying problem with mpfr not being able to find the difference between two different numbers:

sage: c = CIF(0,31)
sage: ci = 1/c
sage: ci.real().endpoints()
(0.000000000000000, -0.000000000000000)
sage: ci.imag().endpoints()
(-0.0322580645161291, -0.0322580645161290)
sage: ci.diameter()
0.000000000000000
sage: ci.imag().endpoints()[1]-ci.imag().endpoints()[0]
0.000000000000000

My guess is that in that particular case, the difference is in the last bit, but one of the numbers uses a higher exponent (both examples I have seen involve the inverses of powers of 2 or powers of two minus one, which are where you would expect to find this kind corner cases), so the precision of the result is set to the bigger exponent, and that last bit of difference is lost.

This problem is exposed in this case because the result of the inversion method is too tight, but I think it would be worth detecting this kind of problems and fixing them, because there are important performance advantages in using tight results.

Contrarily to the (sign, mantissa, exponent) triple, the decimal representation does not tell you the actual value.

sage: x_down = RealField(53, rnd='RNDD')(4648877034705029 * 2**-57)
sage: x_down
0.0322580645161290
sage: x_up = RealField(53, rnd='RNDU')(4648877034705029 * 2**-57)
sage: x_up
0.0322580645161291
sage: x_down == x_up  # these are actually equal!
True
sage: x_down.sign_mantissa_exponent()
(1, 4648877034705029, -57)
sage: x_up.sign_mantissa_exponent()
(1, 4648877034705029, -57)

The two floating points above are the one appearing in our example

sage: c = CIF(0,31)
sage: ci = 1/c
sage: [x.sign_mantissa_exponent() for x in ci.imag().endpoints()]
[(-1, 4648877034705029, -57), (-1, 4648877034705029, -57)]

Copy link

github-actions bot commented May 8, 2024

Documentation preview for this PR (built with commit c687feb; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@videlec videlec force-pushed the revert-cif-fast-inversion-84ab655 branch from 04c87b1 to f6c384e Compare May 8, 2024 08:10
@videlec videlec force-pushed the revert-cif-fast-inversion-84ab655 branch from f6c384e to 1f7c574 Compare May 8, 2024 10:23
@videlec videlec force-pushed the revert-cif-fast-inversion-84ab655 branch from 1f7c574 to c687feb Compare May 8, 2024 13:21
@videlec
Copy link
Contributor Author

videlec commented May 8, 2024

@mezzarobba some doctest failures were due to the reversion. They are fixed in c687feb.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 10, 2024

Reducing to "critical" because "blocker" still doubles as "CI Fix" until this is merged:

@videlec
Copy link
Contributor Author

videlec commented May 10, 2024

Reducing to "critical" because "blocker" still doubles as "CI Fix" until this is merged:

* [CI: Handle the "p: CI Fix" label #37950](https://github.com/sagemath/sage/pull/37950)

Thanks for taking care of this. I would really appreciate this PR to be part of the next beta as it does fix a critical bug!

@mezzarobba
Copy link
Member

Isn't that enough?

Sure, I somehow missed the first of the two tests, sorry!

@vbraun vbraun merged commit 8d59b12 into sagemath:develop May 12, 2024
20 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
unhyperbolic added a commit to unhyperbolic/sage that referenced this pull request 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 pull request 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 pull request 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 pull request 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

Successfully merging this pull request may close these issues.

right_kernel_matrix is wrong over QQbar
6 participants