Opened 4 years ago

Last modified 8 days ago

#24209 needs_review defect

Make `I in QQbar` hold

Reported by: tmonteil Owned by:
Priority: minor Milestone: sage-9.4
Component: coercion Keywords:
Cc: Merged in:
Authors: Thierry Monteil Reviewers: Samuel Lelièvre
Report Upstream: N/A Work issues:
Branch: u/tmonteil/i_in_qqbar_returns_false (Commits, GitHub, GitLab) Commit: 7144ec2b728f878d0a09cf8ec5b541f4459445ca
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

As reported on ​Ask Sage question 39509 (I doesn't belong to QQbar, why?): 

sage: I in QQbar
False

This is because in the __contains__ method of the Parent class, the big try...except fails when evaluating the following: 

sage: bool(I == QQbar(I))
TypeError: unsupported operand parent(s) for +: 'Number Field in I with defining polynomial x^2 + 1' and 'Algebraic Field'

This early fail does not allow further test to return True.

Fixed in #31628. We add a doctest here.

Change History (12)

comment:1 Changed 3 years ago by rws

Probably a duplicate of #17984. You might want to voice your opinion there.

comment:2 Changed 3 years ago by mmezzarobba

The proper way to fix this issue would be via #12715 and #18036. There has been some progress in this direction, but we're not quite there yet.

comment:3 Changed 5 weeks ago by tmonteil

  • Branch set to u/tmonteil/i_in_qqbar_returns_false

comment:4 Changed 5 weeks ago by tmonteil

  • Authors set to Thierry Monteil
  • Commit set to 58b14db855d222c92062b1814bdaaada58acea6d
  • Milestone changed from sage-8.1 to sage-9.3
  • Status changed from new to needs_review

The issue was solved elsewhere, but it is good to prevent future regression with a dedicated doctest.

New commits:

​58b14db#24209 : add doctest for I in QQbar

comment:5 Changed 5 weeks ago by tmonteil

  • Status changed from needs_review to needs_work

Need another doctest for sqrt(-1) which belongs to SR.

comment:6 Changed 5 weeks ago by git

  • Commit changed from 58b14db855d222c92062b1814bdaaada58acea6d to cb580774d0d27341e864f2e902afec0dd166f2e3

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

​cb58077#24209 : add doctest for sqrt(-1) in QQbar

comment:7 Changed 5 weeks ago by tmonteil

  • Status changed from needs_work to needs_review

comment:8 Changed 5 weeks ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4
  • Priority changed from critical to minor

comment:9 Changed 5 weeks ago by slelievre

  • Description modified (diff)
  • Reviewers set to Samuel Lelièvre
  • Status changed from needs_review to positive_review
  • Summary changed from I in QQbar returns False to Make `I in QQbar` hold

I believe this was solved by #31628.

comment:10 follow-up: Changed 9 days ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:11 Changed 8 days ago by git

  • Commit changed from cb580774d0d27341e864f2e902afec0dd166f2e3 to 7144ec2b728f878d0a09cf8ec5b541f4459445ca

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

​7144ec2#24209 : add doctest for I in QQbar and sqrt(-1) in QQbar

comment:12 in reply to: ↑ 10 Changed 8 days ago by tmonteil

  • Status changed from needs_work to needs_review

Replying to vbraun:

Merge conflict

Done.

Note: See TracTickets for help on using tickets.