Opened 8 years ago

Closed 7 years ago

#11904 closed defect (fixed)

Change default variable name for NumberField and NumberFieldElement -> PARI conversion

Reported by: jdemeyer Owned by: davidloeffler
Priority: major Milestone: sage-4.8
Component: number fields Keywords:
Cc: Merged in: sage-4.8.alpha5
Authors: Jeroen Demeyer Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: sage-4.8.alpha2 Stopgaps:

Description (last modified by jdemeyer)

This should be fixed:

sage: K.<a> = NumberField(x^3 + 2)
sage: R.<x> = PolynomialRing(K)
sage: pari(a*x)
---------------------------------------------------------------------------
PariError                                 Traceback (most recent call last)

/usr/local/src/sage-4.7.2.alpha2/<ipython console> in <module>()

/usr/local/src/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/libs/pari/gen.so in sage.libs.pari.gen.PariInstance.__call__ (sage/libs/pari/gen.c:43353)()

/usr/local/src/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/rings/polynomial/polynomial_element.so in sage.rings.polynomial.polynomial_element.Polynomial._pari_ (sage/rings/polynomial/polynomial_element.c:27815)()

/usr/local/src/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/rings/polynomial/polynomial_element.so in sage.rings.polynomial.polynomial_element.Polynomial._pari_with_name (sage/rings/polynomial/polynomial_element.c:28109)()

/usr/local/src/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/libs/pari/gen.so in sage.libs.pari.gen._pari_trap (sage/libs/pari/gen.c:48151)()

PariError:  (5)

The reason is:

sage: K.<a> = NumberField(x^3 + 2)
sage: a._pari_()
Mod(x, x^3 + 2)

Note that the variable x is used. Since polynomials prefer the variable name x, it might be better to use y by default (or any other name) for number field elements.

There is no good way to fix this for relative number fields, so we don't change that.

Also, the _pari_() methods for NumberFieldElement_absolute and NumberFieldElement_relative are exactly the same, so we should avoid code duplication.

Attachments (1)

11904.patch (31.5 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 8 years ago by jdemeyer

  • Summary changed from Change default variable name for NumberFieldElement -> PARI conversion to Change default variable name for NumberField and NumberFieldElement -> PARI conversion

comment:3 Changed 8 years ago by jdemeyer

  • Dependencies changed from #11891 to #11130, #11891 #11890

comment:4 Changed 8 years ago by jdemeyer

  • Dependencies changed from #11130, #11891 #11890 to #11130, #11891, #11890
  • Description modified (diff)

comment:5 Changed 8 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

comment:6 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to doctests in sage/libs/pari

comment:7 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3
  • Status changed from needs_work to needs_review
  • Work issues doctests in sage/libs/pari deleted

comment:8 Changed 7 years ago by jdemeyer

  • Dependencies changed from #11130, #11891, #11890 to #11130, #11321, #11891, #11890, #11836

comment:9 follow-ups: Changed 7 years ago by was

Quick question? What happens if I do:

sage: K.<a> = NumberField(x^3 + 2)
sage: R.<y> = PolynomialRing(K)
sage: pari(a*y)

I would find out for myself, but with sage-4.7.2.alpha3, I get 13 hunks fail, so I can't apply the patch. And I don't know which version of Sage this patch is against.

If the above fails, isn't this a pretty bad approach to this problem? Wouldn't it be better to call the variable in PARI something like "x" or "_x" (say), which is very very unlikely to class. That would use the standard Sage/Python? convention that one should not use stuff prefixed with underscores except in programming.

comment:10 Changed 7 years ago by ohanar

It might make sense to set the default value to depend on the name of the generator (e.g. for instance a -> a_pari or a_parivar).

comment:11 in reply to: ↑ 9 Changed 7 years ago by jdemeyer

Replying to was:

Quick question? What happens if I do:

sage: K.<a> = NumberField(x^3 + 2)
sage: R.<y> = PolynomialRing(K)
sage: pari(a*y)

This fails by raising PariError.

The following does work (and it does not without the patch):

sage: K.<a> = NumberField(x^3 + 2)
sage: R.<y> = PolynomialRing(K)
sage: (a*y)._pari_with_name('x')
Mod(y, y^3 + 2)*x

I would find out for myself, but with sage-4.7.2.alpha3, I get 13 hunks fail, so I can't apply the patch. And I don't know which version of Sage this patch is against.

Well, it should apply against sage-4.7.2.alpha3 (or more recent) if you apply all the dependencies. But it is possible that I forgot a dependency.

If the above fails, isn't this a pretty bad approach to this problem? Wouldn't it be better to call the variable in PARI something like __x or _x (say).

No, this has to do with variable priorities and is not something which is easily fixed. Besides, even if we do this, then you would still have problems again with polynomial rings in multiple variables or relative extensions.

So I'm not claiming this patch fixes all issues, it just improves on the existing behaviour. My patch changes the pari function for number field polynomials from never working to sometimes working (it works if you use x as polynomial variable).

I also think this patch is important for the Sage library, since there we can easily take the PARI variable to be x. For example, my patch changes

Kbnf = self.pari_bnf(proof=proof).nf_subst("'y")
coeffs = [ a._pari_("y") for a in relpoly.coeffs() ]
polrel = pari(coeffs).Polrev()
return Kbnf.rnfisnorminit(polrel)

to the much simpler

Kbnf = self.pari_bnf(proof=proof)
return Kbnf.rnfisnorminit(relpoly._pari_with_name())

comment:12 in reply to: ↑ 9 Changed 7 years ago by jdemeyer

@was: truly fixing the whole Sage<->PARI variable conversion is possible but would require some serious redesigning. Basically, before every computation, we should fix the PARI variables in the correct order.

comment:13 in reply to: ↑ 9 Changed 7 years ago by jdemeyer

Replying to was:

I would find out for myself, but with sage-4.7.2.alpha3, I get 13 hunks fail, so I can't apply the patch. And I don't know which version of Sage this patch is against.

I just tried to apply all dependencies + this patch against sage-4.7.2.alpha4 without any problems at all. Probably you missed some dependency.

comment:14 Changed 7 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:15 Changed 7 years ago by jdemeyer

  • Dependencies changed from #11130, #11321, #11891, #11890, #11836 to #11130, #11321, #11891, #11890, #11836, #11952
  • Milestone set to sage-4.8

comment:16 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Changed 7 years ago by jdemeyer

comment:17 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:18 Changed 7 years ago by jdemeyer

  • Dependencies changed from #11130, #11321, #11891, #11890, #11836, #11952 to sage-4.8.alpha2

comment:19 Changed 7 years ago by cremona

Applies fine to 4.8.alpha4. Testing now. I take the point that this may not be perfect but it's a lot better than it was, so I'll give it a positive review when the tests finish. (Given the author I am not expecting a problem....)

comment:20 Changed 7 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

Happy Christmas!

comment:21 Changed 7 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha5
  • Resolution set to fixed
  • Status changed from positive_review to closed

Happy Christmas to you too, and see you on monday!

Note: See TracTickets for help on using tickets.