Conversion failure ℚ[√a] → CIF

[mezzarobba]

An issue found by Victor Spitzer:

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

Component: numerical

Author: Maarten Derickx

Branch/Commit: public/rings/conversions_to_CIF-23739 @ f2bc7fc

Reviewer: Travis Scrimshaw

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

[koffie]

comment:3

First I thought this was because there was no convert or coerce map registered. But apparently the coercion framework is not even called in this conversion, because the coercion framework works without problems!

sage: NF.<a> = QuadraticField(-2)
sage: CIF.coerce_map_from(NF)
Composite map:
  From: Number Field in a with defining polynomial x^2 + 2
  To:   Complex Interval Field with 53 bits of precision
  Defn:   Generic morphism:
          From: Number Field in a with defining polynomial x^2 + 2
          To:   Complex Lazy Field
          Defn: a -> 1.414213562373095?*I
        then
          Call morphism:
          From: Complex Lazy Field
          To:   Complex Interval Field with 53 bits of precision
sage: CIF.coerce_map_from(NF)(a)
1.414213562373095?*I

[koffie]

comment:4

The problem is that CIF implements its own __call__ function instead of providing an _element_constructor_ method

[koffie]

Branch: u/mderickx/23739

[koffie]

Author: Maarten Derickx

[koffie]

New commits:

d360262

trac #23739: Make ComplexIntervalField use new coercion framework

[koffie]

Commit: d360262

[fchapoton]

comment:6

see patchbot for an error

[sagetrac-git]

Changed commit from d360262 to f5a18fb

[sagetrac-git]

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

f5a18fb

trac #23739 fix doctest

[koffie]

comment:8

Ok, the error is fixed. Sorry for letting that one slip by.

[mezzarobba]

comment:9

Thanks for your work on this ticket!

I think it would be better to remove the __call__() method completely, unless there is a strong reason to keep it.

Also, I realized that I had an old branch lying around where I had started doing something similar (perhaps in view of #15114). I'm not sure why I didn't post it for review; I believe I stumbled upon issues related to #22029 and never got around to recycling the parts that could be. It would probably be worth comparing our approaches and taking the best of both. I don't really have time for that right now (perhaps next week...), but I've pushed my commits to u/mmezzarobba/wip/intervals in case you want to have a look.

[koffie]

comment:10

I agree that removing __call__ would be nicer. In fact I tried to remove __call__ but handling the optional im = parameter further down the chain was getting really messy, and still had some doctest failures that I couldn't figure out how to fix. So in the end I decided this was the cleanest solution. If you figure a nice way to deal with this in _emelent_constructor feel free to do it I just don't know how to do this in a clean way. Also I think the way it works now is also still quite clean, since in the case that im = None the current code behave exactly as if there where no __call__ at all.
P.s. it is not the first place where this happens, look for example at CC.__call__, this has very similar logic.

[koffie]

comment:11

P.s. are you sure you pushed it?

bt-nac-c220:src mderickx$ git pull trac u/mmezzarobba/wip/intervals
fatal: Couldn't find remote ref u/mmezzarobba/wip/intervals

And I don't seem to be able to find it between your other branches as listed on https://github.com/sagemath/sagetrac-mirror/branchesheads

[mezzarobba]

comment:12

Replying to @koffie:

P.s. are you sure you pushed it?

Oops, looks like I didn't indeed. Thanks for the notice!

[tscrim]

comment:13

I disagree with the __call__ sending things up to parent as a wrapped tuple. This makes the conversions from different CIF's slower because of the extra indirection, and I believe we should attempt to keep this a little more optimized. So I think it should immediately return the constructed element when im is not None. (Also, the return ans is superfluous.) Likewise, I disagree with the CC.__call__ implementation, and since the last time that was modified was 2008, it might be good to revisit that code again as well.

If you still feel like the best way forward is to remove the __call__, then I can try to help figure out what is going on and what we can do going forward. I don't understand what you mean by this:

since in the case that im = None the current code behave exactly as if there where no __call__ at all.

Also, any Parent has a default an_element that implements a (very old) caching by calling _an_element_ (if it exists). So that code path could error out in a way that the initial checking is saying it should not.

I would also put the if is_ComplexIntervalField(S): block first, but this is more of my personal preference as the code feels cleaner to me (so feel free to ignore it).

This might also be a good time to convert this from an old-style parent to new-style.

[koffie]

comment:14

Replying to @tscrim:

If you still feel like the best way forward is to remove the __call__, then I can try to help figure out what is going on and what we can do going forward.

Yes, I think removing the __call__ method completely is the way forward. If we do implement a custom __call__ we should really call the __call__ of parent if im = None since otherwise we break coercion. And I solved the reported issue of this ticket by fixing coercion. However removing __call__ completely would be even nicer since then we also have as little as possible overhead when im = None. I personally don't care to much about what happens if im != None so I don't mind calling another function more direct in this case.

I don't understand what you mean by this:

since in the case that im = None the current code behave exactly as if there where no __call__ at all.

What I mean by this is unrelated to performance issues, but more related to behaviour issues. In general one should not overwrite the __call__ method because it breaks all the nice category and coercion stuff in sage. So I made the __call__ method behave as much as possible as if it were not there by making the code behave as if there were no __call__ at all if im = None.

Also, any Parent has a default an_element that implements a (very old) caching by calling _an_element_ (if it exists). So that code path could error out in a way that the initial checking is saying it should not.

I would also put the if is_ComplexIntervalField(S): block first, but this is more of my personal preference as the code feels cleaner to me (so feel free to ignore it).

This might also be a good time to convert this from an old-style parent to new-style.

Yes I agree this is a good time to do this. What kind of things would be involved in this? I guess at least replacing this

ParentWithGens.__init__(self, self._real_field(), ('I',), False, category = Fields())

in the __init__ function of CIF with something else.

[koffie]

comment:15

I looked a bit more into it. And I think that there is no nice way to git rid of __call__ and support the optional im argument, since it will be very difficult to have different code paths depending on wether im is provided or not. The only way I see this happening with the coercion framework is by forcing every coercion morphism into CIF to be a subclass of our own categories.map class which looks ugly.

So maybe I start being in favour of keeping the __call__. So am I correct in understanding that you will be happy if I leave the code if im = None as is and change the behaviour if im != None to:

    return self.element_class(x, im)

[tscrim]

comment:16

Yes, what I am advocating for is this:

def __call__(self, x, im=None):
    if im is None:
        return super(ComplexIntervalField_class,self).__call__(x)
    return complex_interval.ComplexIntervalFieldElement(self, (x, im))

For removing the old-style parent, from a quick look, it looks like the following needs to be done:

• change ParentWithGens to Parent (maybe adding one or two extra methods to keep functionality)
• specify Element = ComplexIntervalFieldElement
• replace explicit calls to ComplexIntervalFieldElement(self, args) by self.element_class(self, args)

I am pretty sure you can use _element_constructor_(self, x, im=None).

Sorry for the shorter responses and not contributing actual code right now, I'm in the process of moving.

[tscrim]

comment:17

Replying to @tscrim:

I am pretty sure you can use _element_constructor_(self, x, im=None).

That was a little too brief. What I mean is that the __call__ should redirect extra arguments to _element_constructor_ in the natural python way. Although I'm still slightly in favor of the special __call__.

[sagetrac-git]

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

074572d	Merge sage 8.1.beta4 into 23739
6d01b5d	Trac #23739: Make CIF into newstyle class

[sagetrac-git]

Changed commit from f5a18fb to 6d01b5d

[koffie]

comment:19

Ok I tried to address all your comments. Made a minor modification to CC as well. I did't turn it into a new style class since it seemed like this would be way more work then for CIF.

[tscrim]

Changed branch from u/mderickx/23739 to public/rings/conversions_to_CIF-23739

[tscrim]

Changed commit from 6d01b5d to f2bc7fc

[tscrim]

comment:20

Great, thank you. Looks good. A good followup will be making this use UniqueRepresentation instead of its own custom cache. However, that will probably be a bit more invasive of a change, so I think it is better as a separate ticket.

I added a little more documentation for _coerce_map_from_ to match what the code does. I also implemented coercions from CC to match the real version:

sage: RIF.coerce_map_from(RR)
Call morphism:
  From: Real Field with 53 bits of precision
  To:   Real Interval Field with 53 bits of precision

The rest of my changes are PEP8 and trivial formatting.

If my changes look good, then positive review.

---

New commits:

5e4ee68	Merge branch 'u/mderickx/23739' of git://trac.sagemath.org/sage into public/rings/conversions_to_CIF-23739
c8c36c5	Some minor reviewer tweaks.
f2bc7fc	Adding CC coercion to match RIF and RR.

[tscrim]

Reviewer: Travis Scrimshaw

[fchapoton]

comment:21

many failing doctests

[tscrim]

comment:22

Most of them should be trivial, but there are a few I don't know what is going on without investigating. Maarten, would you be willing to make the first attempt?

Also, I don't quite understand the change in multi_polynomial.pyx. Can you explain the rationale?

[jdemeyer]

comment:23

I'll try to fix this ticket instead in #24371.

[koffie]

comment:24

Sorry for not following up on this ticket after my initial attempt, thanks for fixing it in #24371. I agree that this then can be seen as duplicate.

[videlec]

comment:25

closing positively reviewed duplicates