Opened 3 years ago

Closed 3 years ago

#23739 closed defect (wontfix)

Conversion failure ℚ[√a] → CIF

Reported by: mmezzarobba Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: numerical Keywords:
Cc: Merged in:
Authors: Maarten Derickx Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: public/rings/conversions_to_CIF-23739 (Commits) Commit: f2bc7fc50cf1338f0cefca315b1c15cd1d6162b9
Dependencies: Stopgaps:

Description (last modified by mmezzarobba)

An issue found by Victor Spitzer:

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

Change History (25)

comment:1 Changed 3 years ago by mmezzarobba

  • Description modified (diff)

comment:2 Changed 3 years ago by mmezzarobba

  • Description modified (diff)

comment:3 Changed 3 years ago by mderickx

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
Last edited 3 years ago by mderickx (previous) (diff)

comment:4 Changed 3 years ago by mderickx

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

Last edited 3 years ago by mderickx (previous) (diff)

comment:5 Changed 3 years ago by mderickx

  • Authors set to Maarten Derickx
  • Branch set to u/mderickx/23739
  • Commit set to d360262afd0929ea9334761ec5035323fa4d665d
  • Status changed from new to needs_review

New commits:

d360262trac #23739: Make ComplexIntervalField use new coercion framework

comment:6 Changed 3 years ago by chapoton

see patchbot for an error

comment:7 Changed 3 years ago by git

  • Commit changed from d360262afd0929ea9334761ec5035323fa4d665d to f5a18fbcdd63848c8744549e86aa442a6c603ab5

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

f5a18fbtrac #23739 fix doctest

comment:8 Changed 3 years ago by mderickx

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

Last edited 3 years ago by mderickx (previous) (diff)

comment:9 Changed 3 years ago by mmezzarobba

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.

comment:10 Changed 3 years ago by mderickx

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.

comment:11 follow-up: Changed 3 years ago by mderickx

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://git.sagemath.org/sage.git/refs/heads

comment:12 in reply to: ↑ 11 Changed 3 years ago by mmezzarobba

Replying to mderickx:

P.s. are you sure you pushed it?

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

comment:13 follow-up: Changed 3 years ago by tscrim

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.

comment:14 in reply to: ↑ 13 Changed 3 years ago by mderickx

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.

Last edited 3 years ago by mderickx (previous) (diff)

comment:15 Changed 3 years ago by mderickx

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)
Last edited 3 years ago by mderickx (previous) (diff)

comment:16 follow-up: Changed 3 years ago by tscrim

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.

Last edited 3 years ago by tscrim (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 3 years ago by tscrim

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__.

comment:18 Changed 3 years ago by git

  • Commit changed from f5a18fbcdd63848c8744549e86aa442a6c603ab5 to 6d01b5d02d4b3c479c7119d5458631aef1341ea6

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

074572dMerge sage 8.1.beta4 into 23739
6d01b5dTrac #23739: Make CIF into newstyle class

comment:19 Changed 3 years ago by mderickx

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.

Last edited 3 years ago by mderickx (previous) (diff)

comment:20 Changed 3 years ago by tscrim

  • Branch changed from u/mderickx/23739 to public/rings/conversions_to_CIF-23739
  • Commit changed from 6d01b5d02d4b3c479c7119d5458631aef1341ea6 to f2bc7fc50cf1338f0cefca315b1c15cd1d6162b9
  • Reviewers set to Travis Scrimshaw

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:

5e4ee68Merge branch 'u/mderickx/23739' of git://trac.sagemath.org/sage into public/rings/conversions_to_CIF-23739
c8c36c5Some minor reviewer tweaks.
f2bc7fcAdding CC coercion to match RIF and RR.

comment:21 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

many failing doctests

comment:22 Changed 3 years ago by tscrim

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?

comment:23 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-8.1 to sage-duplicate/invalid/wontfix

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

comment:24 Changed 3 years ago by mderickx

  • Status changed from needs_work to positive_review

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.

comment:25 Changed 3 years ago by vdelecroix

  • Resolution set to wontfix
  • Status changed from positive_review to closed

closing positively reviewed duplicates

Note: See TracTickets for help on using tickets.