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

Reintroduce tight enclosure of interval inversion #37963

Closed
videlec opened this issue May 8, 2024 · 4 comments · Fixed by #38360
Closed

Reintroduce tight enclosure of interval inversion #37963

videlec opened this issue May 8, 2024 · 4 comments · Fixed by #38360

Comments

@videlec
Copy link
Contributor

videlec commented May 8, 2024

In #37941 the __invert__ implementation of complex intervals was reverted to the straightforward implementation. This issue stands for reintroducing tight reversion (hopefully correct this time).

@miguelmarco @mezzarobba

@videlec
Copy link
Contributor Author

videlec commented May 8, 2024

I did use the following test function.

def check(re, im):
    precs = [6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 31, 32, 33, 100, 127, 128, 129]
    for prec in precs:
        c = ~ComplexIntervalField(prec)(re, im)
        norm2 = re**2 + im**2

        # check real part
        inv = re / norm2
        left, right = c.real().endpoints()
        left = QQ(left.as_integer_ratio())
        right = QQ(right.as_integer_ratio())
        if left > inv or right < inv:
             print('wrong real at prec={}: left={}~{} right={}~{} value={}~{}'.format(prec, left, left.n(prec=2*prec), right, right.n(prec=2*prec), inv, inv.n(prec=2*prec)))

        # check imaginary part
        inv = - im / norm2
        left, right = c.imag().endpoints()
        left = QQ(left.as_integer_ratio())
        right = QQ(right.as_integer_ratio())
        if left > inv or right < inv:
            print('wrong imag at prec={}: left={}~{} right={}~{} value={}~{}'.format(prec, left, left.n(prec=2*prec), right, right.n(prec=2*prec), inv, inv.n(prec=2*prec)))

It seems to be the case that the tight inversion has

  • correct real part always
  • wrong imaginary part often
sage: check(1/2, 1/3)
wrong imag at prec=11: left=-473/512~-0.923828 right=-1891/2048~-0.923340 value=-12/13~-0.923077
wrong imag at prec=12: left=-1891/2048~-0.923340 right=-3781/4096~-0.923096 value=-12/13~-0.923077
wrong imag at prec=17: left=-120991/131072~-0.923088074 right=-60495/65536~-0.923080444 value=-12/13~-0.923076923
sage: check(1, 5)
wrong imag at prec=6: left=-25/128~-0.195 right=-25/128~-0.195 value=-5/26~-0.192
wrong imag at prec=7: left=-99/512~-0.193 right=-99/512~-0.193 value=-5/26~-0.192
wrong imag at prec=8: left=-197/1024~-0.1924 right=-197/1024~-0.1924 value=-5/26~-0.1923
wrong imag at prec=9: left=-197/1024~-0.19238 right=-197/1024~-0.19238 value=-5/26~-0.19231
wrong imag at prec=10: left=-197/1024~-0.19238 right=-197/1024~-0.19238 value=-5/26~-0.19231
wrong imag at prec=11: left=-197/1024~-0.192383 right=-197/1024~-0.192383 value=-5/26~-0.192308
wrong imag at prec=12: left=-3151/16384~-0.192322 right=-3151/16384~-0.192322 value=-5/26~-0.192308
wrong imag at prec=13: left=-3151/16384~-0.1923218 right=-3151/16384~-0.1923218 value=-5/26~-0.1923077
wrong imag at prec=14: left=-3151/16384~-0.19232178 right=-3151/16384~-0.19232178 value=-5/26~-0.19230769
...
sage: check(1/4, -1/2)
wrong imag at prec=6: left=13/8~1.62 right=13/8~1.62 value=8/5~1.60
wrong imag at prec=7: left=103/64~1.61 right=103/64~1.61 value=8/5~1.60
wrong imag at prec=8: left=205/128~1.602 right=205/128~1.602 value=8/5~1.600
wrong imag at prec=9: left=205/128~1.6016 right=205/128~1.6016 value=8/5~1.6000
wrong imag at prec=10: left=205/128~1.6016 right=205/128~1.6016 value=8/5~1.6000
...

@unhyperbolic
Copy link
Contributor

unhyperbolic commented Jul 3, 2024

I started looking at the implementation of the Rokne-Lancaster method that was just removed in 8d59b12 and found a couple of problems:

  1. The following lines should be a rounding down instead of rounding up:
    mpfr_div(imax, c, div1, MPFR_RNDU)

    mpfr_div(aux2, d, div2, MPFR_RNDU)

    Also note that subtracting from 0 can be avoided by simply switching the sign with mpfr_neg (which can even be done in place).
  2. The computation of rmin (the lower bound of the real part of the inverse) does not depend on d (the upper bound of the imaginary part), but it should, consider for example [10,11]+[-3,2], [10,11]+[-3,3], [10,11]+[-3,4]:
    if mpfr_cmp(aux, c) >= 0:
    mpfr_set_str(aux, '-0.5', 10, MPFR_RNDD)
    mpfr_div(rmin, aux, c, MPFR_RNDD)
    else:
    mpfr_mul(a2, a, a, MPFR_RNDD)
    mpfr_mul(c2, c, c, MPFR_RNDD)
    mpfr_add(div1, a2, c2, MPFR_RNDD)
    mpfr_div(rmin, a, div1, MPFR_RNDU)

    Also note that the parsing with mpfr_set_str can be avoided by using mpfr_mul_2si or mpfr_div_2si.

I think I understand the Rokne-Lancaster paper well enough that I can give an implementation a shot.

@unhyperbolic
Copy link
Contributor

Feel free to assign this to me.

@videlec
Copy link
Contributor Author

videlec commented Jul 3, 2024

@unhyperbolic Would be nice if you can provide a (correct) tight enclosure! There is no need for a formal assignment of task. We do not use that much in sagemath.

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
@mkoeppe mkoeppe added this to the sage-10.5 milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants