Opened 9 years ago

Closed 22 months ago

#15114 closed defect (fixed)

disallow dangerous coercions to RIF

Reported by: Marco Streng Owned by:
Priority: major Milestone: sage-9.3
Component: coercion Keywords: RIF RR interval coercion
Cc: Vincent Delecroix, Marc Mezzarobba, Daniel Krenn, Clemens Heuberger, Ben Hutz, John Cremona, Peter Bruin Merged in:
Authors: Marc Mezzarobba Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 3c7644f (Commits, GitHub, GitLab) Commit: 3c7644f483a64f33159a1632e3b2426436a9cf50
Dependencies: Stopgaps:

Status badges

Description (last modified by Marc Mezzarobba)

Remove the following coercions because they make interval arithmetic much less trustworthy.

  • RealField --> RealIntervalField
  • float --> ComplexIntervalField
  • SR --> ComplexIntervalField

For example, one could easily do the following by accident.

    sage: iv = 1 + 2^(-55) + 0. + RIF(1)
    sage: iv.lower(), iv.upper()
    (2.00000000000000, 2.00000000000000)

See also this topic on sage-devel

Already removed in other tickets:

  • ComplexField --> ComplexIntervalField

Change History (54)

comment:1 Changed 9 years ago by Vincent Delecroix

Cc: Vincent Delecroix added
Description: modified (diff)

comment:2 Changed 9 years ago by Marc Mezzarobba

Cc: Marc Mezzarobba added

comment:3 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:4 Changed 9 years ago by Thomas Coffee

I think this change will cause many more problems than it solves.

In practice: I expect this will break a lot of existing code. I often use the RR --> RIF coercion intentionally, because it makes it more convenient to do exactly what I want. For instance:

  • In an interval branch-and-bound code, cell widths in a geometric decomposition are expressed as reciprocal powers of two, which can be efficiently stored and used as EXACT floating-point numbers. Combining these in arithmetic operations with intervals gives the same results as explicitly converting them to RIF, without having to do the explicit conversion.
  • Interval arithmetic is a fast way to obtain approximate bounds on floating-point calculations over ranges of parameters, by simply replacing the parameter of interest with an interval. To do this without RR --> RIF, one would have to add explicit conversions of *all* other parameters appearing in combination with this parameter *everywhere* it occurs.

In theory: I think the motivation is flawed. The change was proposed to avoid accidental coercion of exact ring values to RIF via potentially inexact RR. But the problem in this case is the first step, not the second, and reflects the need for a direct coercion from the exact ring to RIF. In such a case, the lack of such a coercion is the bug. Whereas each value in RR *does* have a valid coercion to RIF, and the lack of this coercion would also be a bug.

Realistically, I think anyone who is actually

  1. using RIF to do reliable computing *and*
  2. coercing values from other exact rings that *don't* have direct coercions to RIF *and* are not expressible exactly in RR,

is going to be sufficiently aware of the potential problem that they will handle/check the conversion explicitly. Better to expect this than to inconvenience many users who use this coercion in far less arcane circumstances.

comment:5 in reply to:  4 Changed 9 years ago by Marco Streng

Replying to tcoffee:

I think this change will cause many more problems than it solves.

Thanks for the comments. I mentioned them in the sage-devel thread where the original discussion took place. Let's move the discussion there.

comment:6 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:7 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:8 Changed 6 years ago by Daniel Krenn

Cc: Daniel Krenn added

comment:9 Changed 6 years ago by Daniel Krenn

Branch: u/dkrenn/remove-coerce-RR-RIF

comment:10 Changed 6 years ago by Marc Mezzarobba

Commit: b30ebc322cd9f6ef887a0e2b123b65b98e9fd1c0

Does your branch pass the tests? I tried doing something similar a while ago (see u/mmezzarobba/wip/coerce_RR_RIF), but I had to make additional changes, and, more importantly, I concluded that this should wait for #22029 if we don't want comparisons between floating-point numbers and intervals to give very misleading results.


New commits:

b30ebc3disallow coercions from RR to RIF

comment:11 in reply to:  10 Changed 6 years ago by Daniel Krenn

Replying to mmezzarobba:

Does your branch pass the tests? I tried doing something similar a while ago (see u/mmezzarobba/wip/coerce_RR_RIF), but I had to make additional changes, and, more importantly, I concluded that this should wait for #22029 if we don't want comparisons between floating-point numbers and intervals to give very misleading results.

I am cleaning up my SageMath-versions and found this partial work. As no branch was attached to this ticket, I've uploaded it. So it will, for sure, need some work.

comment:12 in reply to:  description ; Changed 5 years ago by Jeroen Demeyer

Replying to mstreng:

For example, one could easily do the following by accident.

    sage: iv = 1 + 2^(-55) + 0. + RIF(1)
    sage: iv.lower(), iv.upper()
    (2.00000000000000, 2.00000000000000)

What's wrong with this? I honestly don't really see a problem with coercion RR -> RIF.

comment:13 in reply to:  12 ; Changed 5 years ago by Vincent Delecroix

Replying to jdemeyer:

Replying to mstreng:

For example, one could easily do the following by accident.

    sage: iv = 1 + 2^(-55) + 0. + RIF(1)
    sage: iv.lower(), iv.upper()
    (2.00000000000000, 2.00000000000000)

What's wrong with this? I honestly don't really see a problem with coercion RR -> RIF.

With RIF you always have guaranteed results, with RR you do not. RIF should be as safe as possible to ensure that you actually have proven results. Just compare

sage: r1 = RIF(1/3) + (2.0).cos()
sage: r2 = RIF(1/3) + RIF(2).cos()
sage: r1.endpoints()
(-0.0828135032138091, -0.0828135032138090)
sage: r2.endpoints()
(-0.0828135032138091, -0.0828135032138089)

For me it is similar to the way coercions are implemented between RealField. It goes from more precision to less precision so that you have no artificial big precision in your result.

In conclusion: if any coercion, it should be the other way around (by taking the center).

comment:14 in reply to:  13 ; Changed 5 years ago by Jeroen Demeyer

I think a lot depends on how you model mathematically elements of interval fields. There are two ways to interpret them: either as real numbers with some uncertainty or actually as intervals of real numbers.

In the "interval" model, the coercion RR -> RIF makes sense: you identify a real number with a singleton, which is a special case of an interval. On the other hand, the "choose the middle point map" RIF -> RR is neither natural (the middle point is an arbitrary choice) nor a morphism, which are two requirements for a coercion.

In the "real number with uncertainty" model, you could see RIF -> RR as the forgetful map which keeps the real number but forgets about the uncertainty. In that case, this map is more natural than the opposite.

For me, it is pretty clear that MPFI is really meant to model intervals while arb models real numbers with uncertainty.

comment:15 in reply to:  14 ; Changed 5 years ago by Marc Mezzarobba

Replying to jdemeyer:

For me, it is pretty clear that MPFI is really meant to model intervals

I sort of agree, but still, the key property is that operations on MPFI intervals return over-approximations of the set of possible results (in particular, they round the endpoints of the results in the correct direction), and having a coercion from plain floating-point numbers would effectively break that.

What would make sense to me if you want ”intervals of real numbers” with a coercion from the corresponding real numbers themselves would be a separate parent Intervals(R) explicitly parametrized by the parent R = RR, QQ, SR... where the endpoints live.

comment:16 in reply to:  15 ; Changed 5 years ago by Jeroen Demeyer

Replying to mmezzarobba:

What would make sense to me if you want ”intervals of real numbers”

RealIntervalField is exactly that for RR

with a coercion from the corresponding real numbers themselves

So, is this an argument in favor of the coercion RR -> RIF?

comment:17 in reply to:  16 ; Changed 5 years ago by Marc Mezzarobba

Replying to jdemeyer:

Replying to mmezzarobba:

What would make sense to me if you want ”intervals of real numbers”

RealIntervalField is exactly that for RR

No: operations in RIF round the upper bound of their result up and the lower bound down. They don't just defer to operations in RR.

with a coercion from the corresponding real numbers themselves

So, is this an argument in favor of the coercion RR -> RIF?

Certainly not.

comment:18 in reply to:  17 Changed 5 years ago by Jeroen Demeyer

Replying to mmezzarobba:

Replying to jdemeyer:

Replying to mmezzarobba:

What would make sense to me if you want ”intervals of real numbers”

RealIntervalField is exactly that for RR

No: operations in RIF round the upper bound of their result up and the lower bound down.

Yes, obviously because that's the right thing to do.

They don't just defer to operations in RR.

Technically not, but that is just an implementation detail. RIF is the set of intervals over RR. Whether it's implemented by operations in RR or by a separate library (MPFI in this case) doesn't really matter.

comment:19 in reply to:  13 Changed 5 years ago by Daniel Krenn

Replying to vdelecroix:

What's wrong with this? I honestly don't really see a problem with coercion RR -> RIF.

With RIF you always have guaranteed results, with RR you do not. RIF should be as safe as possible to ensure that you actually have proven results. [...] For me it is similar to the way coercions are implemented between RealField. It goes from more precision to less precision so that you have no artificial big precision in your result.

In conclusion: if any coercion, it should be the other way around (by taking the center).

Huge +1. (Having reliable numerical computations is a (the) major application of interval arithmetic.)

comment:20 Changed 5 years ago by Daniel Krenn

Cc: Clemens Heuberger added

comment:21 in reply to:  14 ; Changed 5 years ago by Marco Streng

Cc: Clemens Heuberger removed

Replying to jdemeyer:

For me, it is pretty clear that MPFI is really meant to model intervals

It is not to me. I found various texts (by MPFI developers) that motivate why MPFI exists and they all start by saying things about "guaranteeing results" and "certified enclosures" for floating point real arithmetic. And if an automatic silent coercion mixes a guaranteed result with a non-guaranteed result, this is bad.

People prove theorems using RealIntervalField as "real numbers that are guaranteed to be in a certain interval", and I would personally trust such theorems much less if they use software that allows automatic accidental coercions from RR to RIF. The reason: a mistake or bug could introduce a (non-guaranteed-correct) element of RR and the coercion will silently add it to your RIF element, which now could have an incorrect error bound.

I did not know that people also use the outward-rounding RIF for "arithmetic with intervals" without caring about guaranteed inclusion. I understand that for them it is convenient that the following works:

sage: I = RIF(pi, 2*pi)
sage: p = RR(pi)
sage: I + p
1.?e1

to get the interval [5, 8]. However, it is hardly any extra work to do the direct thing that the coercion model currently does for you, which is:

sage: I + RIF(p)
1.?e1

And we can also implement additional functions, e.g.

sage: I.translate(p)
sage: I.add(p)

So I am very much in favour of removing automatic coercions between RR and RIF. Perhaps with a DeprecationWarning if possible.

comment:22 in reply to:  21 ; Changed 5 years ago by Daniel Krenn

Replying to mstreng:

So I am very much in favour of removing automatic coercions between RR and RIF. Perhaps with a DeprecationWarning if possible.

If it is considered a bug, then no deprecation is needed...

comment:23 Changed 5 years ago by Marco Streng

Cc: Clemens Heuberger added

comment:24 Changed 5 years ago by Jeroen Demeyer

The problem with this ticket is that various places in Sage use this coercion. In particular QQbar:

**********************************************************************
File "src/sage/rings/number_field/number_field.py", line 1513, in sage.rings.number_field.number_field.NumberField_generic.construction
Failed example:
    K.<a> = NumberField(x^3-5,embedding=0)
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 517, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 920, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.number_field.number_field.NumberField_generic.construction[14]>", line 1, in <module>
        K = NumberField(x**Integer(3)-Integer(5),embedding=Integer(0), names=('a',)); (a,) = K._first_ngens(1)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 529, in NumberField
        return NumberField_version2(polynomial=polynomial, name=name, check=check, embedding=embedding, latex_name=latex_name, assume_disc_small=assume_disc_small, maximize_at_primes=maximize_at_primes, structure=structure)
      File "sage/structure/factory.pyx", line 364, in sage.structure.factory.UniqueFactory.__call__ (build/cythonized/sage/structure/factory.c:1964)
        return self.get_object(version, key, kwds)
      File "sage/structure/factory.pyx", line 403, in sage.structure.factory.UniqueFactory.get_object (build/cythonized/sage/structure/factory.c:2197)
        cache_key = _cache_key(cache_key)
      File "sage/misc/cachefunc.pyx", line 642, in sage.misc.cachefunc.cache_key (build/cythonized/sage/misc/cachefunc.c:3189)
        o = cache_key_unhashable(o)
      File "sage/misc/cachefunc.pyx", line 651, in sage.misc.cachefunc.cache_key_unhashable (build/cythonized/sage/misc/cachefunc.c:3503)
        return tuple(cache_key(item) for item in o)
      File "sage/misc/cachefunc.pyx", line 651, in genexpr (build/cythonized/sage/misc/cachefunc.c:3398)
        return tuple(cache_key(item) for item in o)
      File "sage/misc/cachefunc.pyx", line 642, in sage.misc.cachefunc.cache_key (build/cythonized/sage/misc/cachefunc.c:3189)
        o = cache_key_unhashable(o)
      File "sage/misc/cachefunc.pyx", line 651, in sage.misc.cachefunc.cache_key_unhashable (build/cythonized/sage/misc/cachefunc.c:3503)
        return tuple(cache_key(item) for item in o)
      File "sage/misc/cachefunc.pyx", line 651, in genexpr (build/cythonized/sage/misc/cachefunc.c:3398)
        return tuple(cache_key(item) for item in o)
      File "sage/misc/cachefunc.pyx", line 642, in sage.misc.cachefunc.cache_key (build/cythonized/sage/misc/cachefunc.c:3189)
        o = cache_key_unhashable(o)
      File "sage/misc/cachefunc.pyx", line 653, in sage.misc.cachefunc.cache_key_unhashable (build/cythonized/sage/misc/cachefunc.c:3558)
        k = o._cache_key()
      File "sage/structure/element.pyx", line 1059, in sage.structure.element.Element._cache_key (build/cythonized/sage/structure/element.c:9948)
        return(self.parent(),str(self))
      File "sage/structure/sage_object.pyx", line 237, in sage.structure.sage_object.SageObject.__repr__ (build/cythonized/sage/structure/sage_object.c:2778)
        result = repr_func()
      File "sage/rings/real_lazy.pyx", line 748, in sage.rings.real_lazy.LazyFieldElement._repr_ (build/cythonized/sage/rings/real_lazy.c:9768)
        return str(self.approx())
      File "sage/rings/real_lazy.pyx", line 772, in sage.rings.real_lazy.LazyFieldElement.approx (build/cythonized/sage/rings/real_lazy.c:9863)
        return self.eval(self._parent.interval_field())
      File "sage/rings/real_lazy.pyx", line 1640, in sage.rings.real_lazy.LazyAlgebraic.eval (build/cythonized/sage/rings/real_lazy.c:18796)
        roots = self._poly.roots(ring = AA if isinstance(self._parent, RealLazyField_class) else QQbar)
      File "sage/rings/polynomial/polynomial_element.pyx", line 7540, in sage.rings.polynomial.polynomial_element.Polynomial.roots (build/cythonized/sage/rings/polynomial/polynomial_element.c:68834)
        rts = real_roots(self, retval='algebraic_real')
      File "sage/rings/polynomial/real_roots.pyx", line 4155, in sage.rings.polynomial.real_roots.real_roots (build/cythonized/sage/rings/polynomial/real_roots.c:49563)
        return [(AA.polynomial_root(r[1], r[0]), r[2]) for r in intv_roots]
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/qqbar.py", line 997, in polynomial_root
        return AlgebraicReal(ANRoot(poly, interval, multiplicity))
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/qqbar.py", line 5830, in __init__
        self._interval = self.refine_interval(interval, 64)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/qqbar.py", line 5992, in refine_interval
        return self._real_refine_interval(interval, prec)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/qqbar.py", line 6096, in _real_refine_interval
        new_range = uinfo['endpoint'] - uinfo['value'] / slope
      File "sage/structure/element.pyx", line 1369, in sage.structure.element.Element.__sub__ (build/cythonized/sage/structure/element.c:11538)
        return coercion_model.bin_op(left, right, sub)
      File "sage/structure/coerce.pyx", line 1133, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:10655)
        raise bin_op_exception(op, x, y)
    TypeError: unsupported operand parent(s) for -: 'Real Field with 64 bits of precision and rounding RNDU' and 'Real Interval Field with 64 bits of precision'

comment:25 in reply to:  22 Changed 5 years ago by Jeroen Demeyer

Replying to dkrenn:

If it is considered a bug, then no deprecation is needed...

It's documented behaviour so it's maybe a misfeature but not a bug.

Anyway, it seems that most people think that the coercion RR -> RIF is a misfeature, so I'm willing to remove it in #24371. However, I don't want to deal with the fall-out so if somebody wants to fix 24, please go ahead. It's hard to tell if that's the only issue because that one issue in QQbar causes so many doctest failures.

comment:26 Changed 5 years ago by Jeroen Demeyer

To everybody who is supporting this ticket: please be aware that fixing it will lead to issues like #24410 for RIF too.

comment:27 in reply to:  26 Changed 5 years ago by Vincent Delecroix

Replying to jdemeyer:

To everybody who is supporting this ticket: please be aware that fixing it will lead to issues like #24410 for RIF too.

Obtaining a TypeError for comparison between RR and RIF is perfectly acceptable to me (but somehow annoying).

comment:28 in reply to:  26 Changed 5 years ago by Marc Mezzarobba

Replying to jdemeyer:

To everybody who is supporting this ticket: please be aware that fixing it will lead to issues like #24410 for RIF too.

That what comment #10 above is about.

comment:29 Changed 4 years ago by Marc Mezzarobba

Jeroen: This is the ticket I was talking about at breakfast. I agree that there is an argument for keeping the coercion in place, but I think the cons outweigh the pros, and most people seem to agree. Also, I seem to remember that #21991 and perhaps a couple of other tickets were going to depend on this one, but I don't remember the details.

comment:30 Changed 4 years ago by Jeroen Demeyer

I don't immediately see how this is related to #22029.

comment:31 Changed 4 years ago by Marc Mezzarobba

The link is not immediate. What happens is that

  • before #22029, “interval < float” comparisons gave reasonable (though possibly non-rigorous) results thanks to the coercion that this ticket wants to remove,
  • with the coercion removed but without #22029, they would silently return nonsense (compare by id),
  • they can now (correctly, I'd argue) throw an error if we remove the coercion.

comment:32 Changed 2 years ago by Marc Mezzarobba

Branch: u/dkrenn/remove-coerce-RR-RIFu/mmezzarobba/15114_RR_to_RIF
Commit: b30ebc322cd9f6ef887a0e2b123b65b98e9fd1c0d2e93d76e816c22f85684bca4b161610f37db4f3
Description: modified (diff)
Milestone: sage-6.4sage-9.3
Summary: disallow coercions from RR to RIFdisallow dangerous coercions to RIF

New commits:

e587b14#15114 remove coercions from RealField(n), RDF to RIF
a91e43c#15114 remove coercions from SR, float to CIF
bb9663b#15114 force choice of coercion maps from UCF to CC, CIF, ...
cb7e794#15114 binary_form_reduce(): avoid mixing intervals and FP numbers
df6373a#15114 fix doctest tolerance parsing after removal of RR → RIF coercion
a5488ac#15114 minor fixes to elliptic_curves code and tests mixing RR with RIF
d2e93d7#15114 misc small fixes after removal of dangerous coercions

comment:33 Changed 2 years ago by git

Commit: d2e93d76e816c22f85684bca4b161610f37db4f30aab184224fae9bf51e3f9a5cd6597712d4e977f

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

6b8232c#15114 remove coercions from RR, RDF, float to RIF
c11b6a2#15114 remove coercions from SR, float to CIF
6d7107e#15114 force choice of coercion maps from UCF to CC, CIF, ...
7f7e3b3#15114 binary_form_reduce(): avoid mixing intervals and FP numbers
9d30857#15114 fix doctest tolerance parsing after removal of RR → RIF coercion
6cdc8d6#15114 minor fixes to elliptic_curves code and tests mixing RR with RIF
0aab184#15114 misc small fixes after removal of dangerous coercions

comment:34 Changed 2 years ago by git

Commit: 0aab184224fae9bf51e3f9a5cd6597712d4e977f88e07641ede4a45b6983790d3e36cad5fb0d4885

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

b225ec4#15114 binary_form_reduce: avoid mixing intervals and FP numbers
bd8ac6c#15114 fix doctest tolerance parsing after removal of RR → RIF coercion
ab8ebca#15114 minor fixes to elliptic_curves code and tests mixing RR with RIF
88e0764#15114 misc small fixes after removal of dangerous coercions

comment:35 Changed 2 years ago by Marc Mezzarobba

Authors: Marc Mezzarobba
Cc: Ben Hutz John Cremona Peter Bruin added
Status: newneeds_review

I could not test everything locally due to version issues with pari and giac, so the patchbot may still uncover problems, but this is starting to take shape, and comments are already welcome.

@bhutz: You may want to check the commit touching binary_form_reduce. I don't think I broke anything, but the way intervals are used in the parts I had to modify looks a bit strange to me.

@cremona, @pbruin: Same remark regarding the changes to elliptic_curves.height.

comment:36 Changed 2 years ago by John Cremona

I see no problems with the minor code changes to elliptic curve heights, though I don't think that any of the affected functions was written by me.

comment:37 Changed 2 years ago by Peter Bruin

I don't think I contributed much to this precise code either, but the changes certainly look fine to me.

comment:38 Changed 2 years ago by Marc Mezzarobba

Thank you both for your comments. John: indeed, git blame did not tell the whole story. If I understand correctly, you committed the code but it was originally written by Robert Bradshaw.

comment:39 in reply to:  38 Changed 2 years ago by John Cremona

Replying to mmezzarobba:

Thank you both for your comments. John: indeed, git blame did not tell the whole story. If I understand correctly, you committed the code but it was originally written by Robert Bradshaw.

That agrees with my memory. heegner.py was from his thesis, I think, and he rewrote the unpleasant complex interval stuff from a script I had in (I think) gp.

comment:40 Changed 2 years ago by git

Commit: 88e07641ede4a45b6983790d3e36cad5fb0d48855dcf52cdfc108339d57a326d1c17a7957e34b170

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

5c463c8#15114 remove coercions from RR, RDF, float to RIF
ccf7e2e#15114 remove coercions from SR, float to CIF
c307736#15114 force choice of coercion maps from UCF to CC, CIF, ...
97a48a2#15114 binary_form_reduce: avoid mixing intervals and FP numbers
02308be#15114 fix doctest tolerance parsing after removal of RR → RIF coercion
7e0d6d9#15114 minor fixes to elliptic_curves code and tests mixing RR with RIF
5dcf52c#15114 misc small fixes after removal of dangerous coercions

comment:41 Changed 2 years ago by Ben Hutz

The changes in binary_form_reduce.py look fine to me. The use of .lower() and .upper() I expect should have been there in the first place and we've just never come across an example where being that careful with the errors mattered.

comment:42 in reply to:  41 Changed 2 years ago by Marc Mezzarobba

Replying to bhutz:

The changes in binary_form_reduce.py look fine to me. The use of .lower() and .upper() I expect should have been there in the first place and we've just never come across an example where being that careful with the errors mattered.

Thank you.

All tests pass now, reviewers welcome!

comment:43 Changed 2 years ago by Vincent Delecroix

This is great. I am positively surprised for seeing only few changes for making this happen!

The only trouble I see is that it might break user code without deprecation notice.

comment:44 in reply to:  43 ; Changed 2 years ago by Marc Mezzarobba

Replying to vdelecroix:

This is great. I am positively surprised for seeing only few changes for making this happen!

The switch to Python 3 (and, I think, also the change to make i an element of ℚ[i] that you recently reviewed) went a long way toward solving the worst issues I found the first time I tried. But it is good news in any case!

The only trouble I see is that it might break user code without deprecation notice.

I agree. But I don't know what to to to ease the transition. Do you see a way to display a deprecation warning when a coercion map is used while keeping the corresponding conversion map working?

That being said, between the issues it indirectly causes and the inconsistency with other interval fields, I would almost call the existence of this coercion a bug.

comment:45 in reply to:  44 ; Changed 2 years ago by Vincent Delecroix

Replying to mmezzarobba:

Replying to vdelecroix:

This is great. I am positively surprised for seeing only few changes for making this happen!

The switch to Python 3 (and, I think, also the change to make i an element of ℚ[i] that you recently reviewed) went a long way toward solving the worst issues I found the first time I tried. But it is good news in any case!

The only trouble I see is that it might break user code without deprecation notice.

I agree. But I don't know what to to to ease the transition. Do you see a way to display a deprecation warning when a coercion map is used while keeping the corresponding conversion map working?

One possibility would be to support the operations (with a warning) without having the coercions. One cumbersome way to do this is to implement custom __add__, __mul__, etc

class Interval:
    def __add__(self, other):
        if self is an interval and other a floating point:
            warn("don't do that")
            return old_behavior(self, other)
        else:
            return Element.__add__(self, other)

But I don't see a simple way to do it in order to also handle functorial constructions such as vectors or polynomials. Though, given the changes you had to do in the doctests, I think it makes sense to only consider scalars.

That being said, between the issues it indirectly causes and the inconsistency with other interval fields, I would almost call the existence of this coercion a bug.

Agreed, but https://xkcd.com/1172/

comment:46 in reply to:  45 ; Changed 2 years ago by Marc Mezzarobba

Replying to vdelecroix:

One possibility would be to support the operations (with a warning) without having the coercions. One cumbersome way to do this is to implement custom __add__

Unless we do it for floating-point types too (which would be a bit invasive), it will work for interval + float but float + interval will still fail.

comment:47 Changed 2 years ago by git

Commit: 5dcf52cdfc108339d57a326d1c17a7957e34b170ddab8474d3a942a18188515eae80175d07ee1c5a

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

315ddc2#15114 remove coercions from RR, RDF, float to RIF
3cc998a#15114 remove coercions from SR, float to CIF
a51cfe0#15114 force choice of coercion maps from UCF to CC, CIF, ...
ccd98a7#15114 binary_form_reduce: avoid mixing intervals and FP numbers
ca84412#15114 fix doctest tolerance parsing after removal of RR → RIF coercion
584d6e4#15114 minor fixes to elliptic_curves code and tests mixing RR with RIF
e1aabd6#15114 misc small fixes after removal of dangerous coercions
ddab847#15114 restore basic functionality, with deprecation warnings

comment:48 in reply to:  46 Changed 2 years ago by Marc Mezzarobba

Replying to mmezzarobba:

Replying to vdelecroix:

One possibility would be to support the operations (with a warning) without having the coercions. One cumbersome way to do this is to implement custom __add__

Unless we do it for floating-point types too (which would be a bit invasive), it will work for interval + float but float + interval will still fail.

Ok, for lack of a better option, I have done that—for RealNumbers only. It was... painful.

comment:49 Changed 23 months ago by git

Commit: ddab8474d3a942a18188515eae80175d07ee1c5a3c7644f483a64f33159a1632e3b2426436a9cf50

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

674b4f7#15114 remove coercions from RR, RDF, float to RIF
9abccbb#15114 remove coercions from SR, float to CIF
3be1c0f#15114 force choice of coercion maps from UCF to CC, CIF, ...
5fce3b6#15114 binary_form_reduce: avoid mixing intervals and FP numbers
ee5fd27#15114 fix doctest tolerance parsing after removal of RR → RIF coercion
ba66ce3#15114 minor fixes to elliptic_curves code and tests mixing RR with RIF
9f47f4c#15114 misc small fixes after removal of dangerous coercions
3c7644f#15114 restore basic functionality, with deprecation warnings

comment:50 Changed 23 months ago by Marc Mezzarobba

Rebased to fix a merge conflict due to whitespace changes...

comment:51 Changed 23 months ago by Marc Mezzarobba

Vincent, do you think you will finish the review, or should I look for someone willing to take over?

comment:52 Changed 23 months ago by Vincent Delecroix

Reviewers: Vincent Delecroix
Status: needs_reviewpositive_review

comment:53 Changed 23 months ago by Marc Mezzarobba

Great, thank you!

comment:54 Changed 22 months ago by Volker Braun

Branch: u/mmezzarobba/15114_RR_to_RIF3c7644f483a64f33159a1632e3b2426436a9cf50
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.