Move real/complex interval fields to new coercion model

[jdemeyer]

Also fix the following:

sage: NF.<a> = QuadraticField(-2)
sage: CIF(a)
...
ValueError: can not convert complex algebraic number to real interval

We also deal with Python 3 compatibility regarding int/long and bytes/unicode.

Depends on #24423

CC: @tscrim

Component: coercion

Author: Jeroen Demeyer

Branch/Commit: 9ac1612

Reviewer: Travis Scrimshaw

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

[jdemeyer]

Branch: u/jdemeyer/move_real_complex_interval_fields_to_new_coercion_model

[sagetrac-git]

Commit: db4611a

[sagetrac-git]

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

375e40f	Map._extra_slots(): do not pass dict
db4611a	Move real/complex interval fields to new coercion model

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

Dependencies: #24372

[sagetrac-git]

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

549bd7d

Move real/complex interval fields to new coercion model

[sagetrac-git]

Changed commit from db4611a to 549bd7d

[sagetrac-git]

Changed commit from 549bd7d to fe4c823

[sagetrac-git]

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

fe4c823

Move real/complex interval fields to new coercion model

[sagetrac-git]

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

2cee3a4

Move real/complex interval fields to new coercion model

[sagetrac-git]

Changed commit from fe4c823 to 2cee3a4

[tscrim]

comment:9

This will conflict with #23739, but note that #23739 is currently stalled.

[tscrim]

comment:10

Also, I imagine #24363 is a dependency from looking at the code.

[jdemeyer]

comment:11

Replying to @tscrim:

Also, I imagine #24363 is a dependency from looking at the code.

I don't see why.

[jdemeyer]

comment:13

Replying to @tscrim:

This will conflict with #23739, but note that #23739 is currently stalled.

I shouldn't be hard to fix #23739 in this ticket.

[koffie]

comment:14

I also shortly want to note #15114 here and the discussion from which that ticket spawned at sage-devel. I am not saying it needs to get fixed here, but it is a good thing to have in the back of your mind while working on this ticket so that at least things do not get worse in that respect.

[tscrim]

comment:15

Replying to @jdemeyer:

Replying to @tscrim:

Also, I imagine #24363 is a dependency from looking at the code.

I don't see why.

I thought I saw a conflict, but that turns out to not be the case.

[sagetrac-git]

Changed commit from 2cee3a4 to d66b54f

[sagetrac-git]

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

d66b54f

Move real/complex interval fields to new coercion model

[jdemeyer]

comment:17

Replying to @koffie:

I also shortly want to note #15114 here and the discussion from which that ticket spawned at sage-devel. I am not saying it needs to get fixed here, but it is a good thing to have in the back of your mind while working on this ticket so that at least things do not get worse in that respect.

OK. So with #15114 in mind, this should be a feature and not a bug:

sage: CC.one() + CIF.one()
...
TypeError: unsupported operand parent(s) for +: 'Complex Field with 53 bits of precision' and 'Complex Interval Field with 53 bits of precision'

[sagetrac-git]

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

3eaf54f	Clean up MPFI declarations
4984485	Move real/complex interval fields to new coercion model

[sagetrac-git]

Changed commit from d66b54f to 4984485

[sagetrac-git]

Changed commit from 4984485 to 2448563

[sagetrac-git]

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

[jdemeyer]

comment:37

I still need to implement the Python 3 compatibility that I promised.

[sagetrac-git]

Changed commit from c74d8e7 to 1195b3a

[sagetrac-git]

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

1195b3a

Move real/complex interval fields to new coercion model

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:40

Some comments:

• I know this is slightly outside the purview of this ticket, but perhaps we can rename ComplexIntervalField_class to ComplexIntervalField and have it inherit from UniqueRepresentation in order to get rid of the custom caching. I can also do this if you want.

• I would do the same for RealIntervalField_class except it is an extension class. Do you think there is a benefit for that? We don't even do that for Rationals.

• Why not use @cached_method for real_field, middle_field, etc.? They to have UniqueRepresentation-like behavior, so do not strictly need to be cached. However, I am assuming they are cached for speed reasons, so doing @cached_method is the best for repeated use:

  sage: class Foo(object):
  ....:     def __init__(self):
  ....:         self._cache2 = None
  ....:     @cached_method
  ....:     def cm(self):
  ....:         return 5
  ....:     def c1(self):
  ....:         try:
  ....:             return self._cache1
  ....:         except AttributeError:
  ....:             self._cache1 = 5
  ....:             return self._cache1
  ....:     def c2(self):
  ....:         if self._cache2 is None:
  ....:             self._cache2 = 5
  ....:         return self._cache2
  sage: timeit('F = Foo(); F.cm()', number=10000, repeat=20)
  10000 loops, best of 20: 1.53 µs per loop
  sage: timeit('F = Foo(); F.c1()', number=10000, repeat=20)
  10000 loops, best of 20: 1.39 µs per loop
  sage: timeit('F = Foo(); F.c2()', number=10000, repeat=20)
  10000 loops, best of 20: 635 ns per loop
  sage: F = Foo()
  sage: timeit('F.cm()', number=10000, repeat=20)
  10000 loops, best of 20: 68.7 ns per loop
  sage: timeit('F.c1()', number=10000, repeat=20)
  10000 loops, best of 20: 121 ns per loop
  sage: timeit('F.c2()', number=10000, repeat=20)
  10000 loops, best of 20: 1.49 µs per loop

• Is the _real_field = real_field leftover from testing? I think it should be removed altogether.

• You should replace

        return complex_interval.ComplexIntervalFieldElement(self, x, im, **kwds)
        return self.element_class(self, x, im, **kwds)

  (in fact, if that is used elsewhere in the file, it should be replaced as well) to pick up the category contributions.

• Same comment for returning RealIntervalFieldElement.

• Similarly, I would also make this change:

       R = RealIntervalField(prec=max(bits+pad, min_prec))
       if upper is not None:
           s = (s, upper)
  -    return RealIntervalFieldElement(R, s, base)
  +    return R.element_class(R, s, base)

I think it looks good otherwise.

[jdemeyer]

comment:41

I consider the proposed changes to unique representation outside the scope of this ticket. This ticket is already reasonably large, let's not add more things. I'll have a look at your other comments.

[tscrim]

comment:42

I guess the _real_field = real_field is to avoid #24695 as a dependency?

[tscrim]

comment:43

We should also do changes of the form in ComplexIntervalFieldElement:

-return ComplexIntervalFieldElement(self._parent, re, im)
+return type(self)(self._parent, re, im)

and similarly for RealIntervalFieldElement.

[jdemeyer]

comment:44

Replying to @tscrim:

I guess the _real_field = real_field is to avoid #24695 as a dependency?

I wasn't actually aware of that, thanks for the pointer :-)

[jdemeyer]

comment:45

I guess I kept _real_field for compatibility with other "complex number" implementations such as

sage: CC._real_field()
Real Field with 53 bits of precision

We should probably have a separate ticket to replace _real_field by real_field: this looks like a useful method, it should not be private.

[sagetrac-git]

Changed commit from 1195b3a to 22d309c

[sagetrac-git]

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

22d309c

Cleanup for real/complex interval fields

[jdemeyer]

comment:47

I ended up doing a lot of cleanup. I think I'm using element_class everywhere, but don't shoot me if I'm wrong.

Note that currently the category stuff won't be used anyway because the elements are extension types.

[tscrim]

comment:48

Thank you for doing the cleanup.

For changes such as

-        cdef ComplexIntervalFieldElement res = self._new()
+        res = self._new()

aren't we loosing some efficiency? I would think because they are still subclasses of ComplexIntervalFieldElement, that Cython would be able to optimize their attribute lookups by not treating them as Python objects? (Of course, we would have to explicitly cast <ComplexIntervalFieldElement> to avoid the type-checking-safety-penalty.)

This goes through the __call__ (which in this case, bypasses the coercion framework, but normally it wouldn't):

-        return complex_interval.ComplexIntervalFieldElement(self, 0, 1)
+        return self(0, 1)

I think we are better making a direct call to self.element_class(self, 0, 1) since this is not user input.

[jdemeyer]

comment:49

Replying to @tscrim:

Thank you for doing the cleanup.

For changes such as

-        cdef ComplexIntervalFieldElement res = self._new()
+        res = self._new()

aren't we loosing some efficiency?

No, Cython has type inference. Since _new is declared as returning a ComplexIntervalFieldElement, the declaration cdef ComplexIntervalFieldElement res is just redundant.

This goes through the __call__ (which in this case, bypasses the coercion framework, but normally it wouldn't):

-        return complex_interval.ComplexIntervalFieldElement(self, 0, 1)
+        return self(0, 1)

I think we are better making a direct call to self.element_class(self, 0, 1) since this is not user input.

I don't see the problem with using __call__ here but I can make that change.

[sagetrac-git]

Changed commit from 22d309c to 9ac1612

[sagetrac-git]

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

9ac1612

Cleanup for real/complex interval fields

[jdemeyer]

comment:51

Replying to @tscrim:

Cython would be able to optimize their attribute lookups by not treating them as Python objects?

Actually, the relevant attributes are cdef attributes, so they cannot even be accessed from Python! So the fact that the code works proves that Cython must be doing type inference.

I changed some self(...) calls to self.element_class(self, ...)

[tscrim]

comment:52

Replying to @jdemeyer:

Replying to @tscrim:

Cython would be able to optimize their attribute lookups by not treating them as Python objects?

Actually, the relevant attributes are cdef attributes, so they cannot even be accessed from Python! So the fact that the code works proves that Cython must be doing type inference.

That is what is happening, e.g.:

  __pyx_t_1 = ((PyObject *)__pyx_f_4sage_5rings_16complex_interval_27ComplexIntervalFieldE
lement__new(__pyx_v_self)); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 325, __pyx_L1_error)
  __Pyx_GOTREF(__pyx_t_1);
  __pyx_v_a00 = ((struct __pyx_obj_4sage_5rings_16complex_interval_ComplexIntervalFieldEle
ment *)__pyx_t_1);
  __pyx_t_1 = 0;

I didn't realize that Cython was that smart. :)

I changed some self(...) calls to self.element_class(self, ...)

Thank you. LGTM. (If I wanted to be really picky, I would say self.element_class(self, 1) should instead use the cached self.one(), but zeta(1) is almost certainly never called.)

[jdemeyer]

comment:53

Thanks!

[vbraun]

Changed branch from public/24371-move_real_complex_interval_fields_to_new_coercion_model to 9ac1612