Opened 6 years ago
Closed 6 years ago
#20064 closed defect (fixed)
Bug in sqrt in QQbar
Reported by:  cremona  Owned by:  

Priority:  major  Milestone:  sage7.1 
Component:  numerical  Keywords:  
Cc:  Merged in:  
Authors:  Nils Bruin  Reviewers:  John Cremona 
Report Upstream:  N/A  Work issues:  
Branch:  48c12ef (Commits, GitHub, GitLab)  Commit:  48c12efbf46cbd4143b1c605004bd6224461b3d8 
Dependencies:  Stopgaps: 
Description
See #18836. This bug is holding up that (and also #20028). The following code creates an elemnt d of QQbar and tries to do d.sqrt(). It fails unless you call d.imag().is_zero() first.
K.<i> = QuadraticField(1) # define a lowprecision embedding from K to CC: emb = K.embeddings(CC)[1] # extend this to the closest embedding into QQbar: old_gen = emb(K.gen()) rr = K.defining_polynomial().roots(QQbar, multiplicities=False) diffs = [(CC(r)old_gen).abs() for r in rr] new_gen = rr[diffs.index(min(diffs))] emb0 = K.hom([new_gen], check=False) # Take a polynomial with 3 roots in K: e1 = 4+i e2 = 1+i e3 = 32*i print("Original ei: %s with parent %s" % ([e1,e2,e3],parent(e1))) x = polygen(K) pol = (xe1)*(xe2)*(xe3) # Find the roots again in QQbar: pol0 = PolynomialRing(QQbar,'x')([emb0(c) for c in list(pol)]) e1, e2, e3 = pol0.roots(QQbar,multiplicities=False) print("Roots ei: %s with parent %s" % ([e1,e2,e3],parent(e1))) # Attempt to compute sqrt(e1e2) from these: d = e1e2 print("d=%s with parent %s" % (d,d.parent())) # If the next 2 lines are commented out, an error is raised in the sqrt! s = d.imag().is_zero() print("d.imag().is_zero()=%s" % s) print("d=%s with parent %s" % (d,d.parent())) d = d.sqrt() print("d=%s" % d)
Change History (7)
comment:1 Changed 6 years ago by
comment:2 followup: ↓ 3 Changed 6 years ago by
Surely it is simplest to implement arg for RIF elements, returning 0 or Pi depending on sign provided that the interval has constant sign, and raising an error otherwise?
comment:3 in reply to: ↑ 2 Changed 6 years ago by
Replying to cremona:
Surely it is simplest to implement arg for RIF elements, returning 0 or Pi depending on sign provided that the interval has constant sign, and raising an error otherwise?
I would regard that as a "change in design/API", though. I think our real fields currently quite consistently do not have an "argument" method:
sage: CC(4).argument() 3.14159265358979 sage: RR(4).argument() AttributeError: ... sage: CDF(4).argument() 3.141592653589793 sage: RDF(4).argument() AttributeError: ...
Your proposal is in line with the solution taken in #18337 (to put "real" and "imag" on RIF) so perhaps putting "argument" there is the simplest solution.
From an efficiency point of view: I'm not sure how hard we'd be hitting the coercion system by mixing RIF and CIF elements.
comment:4 Changed 6 years ago by
 Branch set to u/nbruin/bug_in_sqrt_in_qqbar
comment:5 Changed 6 years ago by
 Commit set to 48c12efbf46cbd4143b1c605004bd6224461b3d8
 Component changed from number fields to numerical
 Status changed from new to needs_review
New commits:
48c12ef  trac #20064: introduce "argument" on RealIntervalFieldElement

comment:6 Changed 6 years ago by
 Reviewers set to John Cremona
 Status changed from needs_review to positive_review
This looks good to me. I have checked that it deals with the original problems I had at #18836, so I am going to set this to positive_review, make that ticket depend on this and set that one to needs_review.
comment:7 Changed 6 years ago by
 Branch changed from u/nbruin/bug_in_sqrt_in_qqbar to 48c12efbf46cbd4143b1c605004bd6224461b3d8
 Resolution set to fixed
 Status changed from positive_review to closed
(Extracted from #18836 comment 9)