Opened 12 months ago
Closed 10 months ago
#27646 closed defect (fixed)
totallyreal_rel.py call to polredabs
Reported by:  jvoight  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  packages: standard  Keywords:  pari, totallyreal, number fields 
Cc:  benjamin.linowitz@…  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  John Voight 
Report Upstream:  Reported upstream. Developers deny it's a bug.  Work issues:  
Branch:  3090d7b (Commits)  Commit:  3090d7bda2a9eced93a8972b3c36062cc5483f89 
Dependencies:  Stopgaps: 
Description (last modified by )
Reported by Ben Linowitz. Running
enumerate_totallyreal_fields_all(6,10^6)
throws an error:
 PariError Traceback (most recent call last) <ipythoninput2a56c359e898e> in <module>() > 1 enumerate_totallyreal_fields_all(Integer(6),Integer(10)**Integer(6),verbose=True) /home/jvoight/sage8.2/local/lib/python2.7/sitepackages/sage/misc/lazy_import.pyx in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3756)() 352 True 353 """ > 354 return self.get_object()(*args, **kwds) 355 356 def __repr__(self): /home/jvoight/sage8.2/local/lib/python2.7/sitepackages/sage/rings/number_field/totallyreal_rel.py in enumerate_totallyreal_fields_all(n, B, verbose, return_seqs, return_pari_objects) 963 print("Taking F =", Sds[i][1]) 964 F = NumberField(ZZx(Sds[i][1]), 't') > 965 T = enumerate_totallyreal_fields_rel(F, n/d, B, verbose=verbose, return_seqs=return_seqs) 966 if return_seqs: 967 for i in range(4): /home/jvoight/sage8.2/local/lib/python2.7/sitepackages/sage/rings/number_field/totallyreal_rel.py in enumerate_totallyreal_fields_rel(F, m, B, a, verbose, return_seqs, return_pari_objects) 794 # Find a minimal lattice element 795 counts[3] += 1 > 796 ng = pari([nf,zk]).polredabs() 797 798 # Check if K is contained in the list. cypari2/auto_gen.pxi in cypari2.gen.Gen_auto.polredabs() cypari2/handle_error.pyx in cypari2.handle_error._pari_err_handle() PariError: incorrect type in nfbasic_init (t_VEC)
The problem is that the polynomial nf
is not monic, but it should be.
Reported upstream: https://pari.math.ubordeaux.fr/cgibin/bugreport.cgi?bug=2136 (not a bug)
Change History (23)
comment:1 Changed 12 months ago by
 Cc benjamin.linowitz@… added; Benjamin Linowitz <benjamin.linowitz@…> removed
comment:2 in reply to: ↑ description Changed 12 months ago by
comment:3 Changed 12 months ago by
 Component changed from interfaces to packages: standard
 Description modified (diff)
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:4 Changed 12 months ago by
Could be worth checking whether this still occurs with the branch at #27605.
comment:5 Changed 12 months ago by
 Description modified (diff)
 Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers deny it's a bug.
I already checked but it's really unrelated to the trashcan. It just happens by pure coincidence that the same line of code is affected.
comment:6 Changed 12 months ago by
Thank you very much! OK, this code to enumerate totally real fields only constructs monic polynomialsand the code used to run just finewhich means something else changed. One basic worry is that the coercion from vectors to polynomials is done in a different order, or something else is wrong...
comment:7 Changed 12 months ago by
I think the issue is in line 778 of rings/number_field/totallyreal_rel.py: it computes a polynomial resultant (to go from the relative field to the absolute field), in this case the resultant of x^2 + (t  1)*x  1
and t^3  t^2  2*t + 1
with respect to t
, which gives as output x^6 + 2*x^5 + 4*x^4  5*x^3  4*x^2 + 2*x + 1
which is not monic and therefore makes pari's nfbasis
unhappy. By construction, the polynomials entering into polresultant
are monic (and integral), so the output should be monic up to sign. I understand there are certain conventions with respect to resultants, so the best thing would be to add something in between lines 778 and 779 like
if nf[n] == 1: nf *= 1
comment:8 Changed 11 months ago by
 Branch set to u/chapoton/27646
 Commit set to 81e131b02facddbabc157d9e9ec7cef83d450959
 Status changed from new to needs_review
New commits:
81e131b  trac 27646 fix in enumeration of totally real fields

comment:9 Changed 11 months ago by
Thanks!
There were too many changes for me to review this quicklylooks like it was changes in spacing and formatting, so I'm not sure where the content was changed.
I did confirm that the fix I suggested in line 778 is there, but I didn't test all of the changes.
comment:10 Changed 11 months ago by
Yes, sorry for this "formatting noise", I just could not resist fixing that.
The only serious change is
 nf = nf.polresultant(nfF, parit) + nf = nf.polresultant(nfF, parit) # either monic or monic + if nf[n] == 1: + nf *= 1
comment:11 Changed 11 months ago by
One should add a doctest, but if possible a shorttime one, not the original bug which seems to take many seconds.
comment:12 Changed 11 months ago by
comment:13 Changed 11 months ago by
possible smaller doctest:
sage: enumerate_totallyreal_fields_all(6,435000)
failing before the ticket just as the original doctest
comment:14 Changed 11 months ago by
 Status changed from needs_review to needs_work
OK, please add that doctest then.
comment:15 Changed 11 months ago by
Well, still takes 5 seconds, which is a lot...
File "src/sage/rings/number_field/totallyreal_rel.py", line 77, in sage.rings.number_field.totallyreal_rel Warning, slow doctest: L = enumerate_totallyreal_fields_all(6,435000) # long time Test ran for 4.90 s
comment:16 Changed 11 months ago by
It's OK for a # long time
test.
comment:17 Changed 11 months ago by
 Commit changed from 81e131b02facddbabc157d9e9ec7cef83d450959 to 3090d7bda2a9eced93a8972b3c36062cc5483f89
Branch pushed to git repo; I updated commit sha1. New commits:
3090d7b  add doctest (5 seconds)

comment:18 Changed 11 months ago by
ok, doctest added and tagged
comment:19 Changed 10 months ago by
review, someone ?
comment:20 Changed 10 months ago by
 Status changed from needs_work to positive_review
comment:21 Changed 10 months ago by
 Status changed from positive_review to needs_work
The reviewer name is missing
comment:22 Changed 10 months ago by
 Reviewers set to John Voight
 Status changed from needs_work to positive_review
comment:23 Changed 10 months ago by
 Branch changed from u/chapoton/27646 to 3090d7bda2a9eced93a8972b3c36062cc5483f89
 Resolution set to fixed
 Status changed from positive_review to closed
Replying to jvoight:
I think it's the same code by accident. The error looks unrelated.