#27646 closed defect (fixed)

totallyreal_rel.py call to polredabs

Reported by: jvoight Owned by:
Priority: major Milestone: sage-8.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 jdemeyer)

Reported by Ben Linowitz. Running

enumerate_totallyreal_fields_all(6,10^6)

throws an error:

---------------------------------------------------------------------------
PariError                                 Traceback (most recent call last)
<ipython-input-2-a56c359e898e> in <module>()
----> 1 enumerate_totallyreal_fields_all(Integer(6),Integer(10)**Integer(6),verbose=True)

/home/jvoight/sage-8.2/local/lib/python2.7/site-packages/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/sage-8.2/local/lib/python2.7/site-packages/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/sage-8.2/local/lib/python2.7/site-packages/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.u-bordeaux.fr/cgi-bin/bugreport.cgi?bug=2136 (not a bug)

Change History (23)

comment:1 Changed 12 months ago by jvoight

  • Cc benjamin.linowitz@… added; Benjamin Linowitz <benjamin.linowitz@…> removed

comment:2 in reply to: ↑ description Changed 12 months ago by jdemeyer

Replying to jvoight:

Is it possibly related to the closed issue #27267? It's precisely the same line in the code.

I think it's the same code by accident. The error looks unrelated.

comment:3 Changed 12 months ago by jdemeyer

  • 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 slelievre

Could be worth checking whether this still occurs with the branch at #27605.

comment:5 Changed 12 months ago by jdemeyer

  • 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 jvoight

Thank you very much! OK, this code to enumerate totally real fields only constructs monic polynomials--and the code used to run just fine--which 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 jvoight

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 chapoton

  • Branch set to u/chapoton/27646
  • Commit set to 81e131b02facddbabc157d9e9ec7cef83d450959
  • Status changed from new to needs_review

New commits:

81e131btrac 27646 fix in enumeration of totally real fields

comment:9 Changed 11 months ago by jvoight

Thanks!

There were too many changes for me to review this quickly--looks 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 chapoton

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 chapoton

One should add a doctest, but if possible a short-time one, not the original bug which seems to take many seconds.

comment:12 Changed 11 months ago by chapoton

  • Authors set to Frédéric Chapoton

comment:13 Changed 11 months ago by chapoton

possible smaller doctest:

sage: enumerate_totallyreal_fields_all(6,435000)

failing before the ticket just as the original doctest

Last edited 11 months ago by chapoton (previous) (diff)

comment:14 Changed 11 months ago by jdemeyer

  • Status changed from needs_review to needs_work

OK, please add that doctest then.

comment:15 Changed 11 months ago by chapoton

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 jdemeyer

It's OK for a # long time test.

comment:17 Changed 11 months ago by git

  • Commit changed from 81e131b02facddbabc157d9e9ec7cef83d450959 to 3090d7bda2a9eced93a8972b3c36062cc5483f89

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

3090d7badd doctest (5 seconds)

comment:18 Changed 11 months ago by chapoton

ok, doctest added and tagged

comment:19 Changed 10 months ago by chapoton

review, someone ?

comment:20 Changed 10 months ago by jvoight

  • Status changed from needs_work to positive_review

comment:21 Changed 10 months ago by vbraun

  • Status changed from positive_review to needs_work

The reviewer name is missing

comment:22 Changed 10 months ago by chapoton

  • Reviewers set to John Voight
  • Status changed from needs_work to positive_review

comment:23 Changed 10 months ago by vbraun

  • Branch changed from u/chapoton/27646 to 3090d7bda2a9eced93a8972b3c36062cc5483f89
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.