Opened 11 years ago

Closed 11 years ago

#11904 closed defect (fixed)

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

Reported by: Jeroen Demeyer Owned by: David Loeffler
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:

Status badges

Description (last modified by Jeroen Demeyer)

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 Jeroen Demeyer 11 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 11 years ago by Jeroen Demeyer

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

comment:3 Changed 11 years ago by Jeroen Demeyer

Dependencies: #11891#11130, #11891 #11890

comment:4 Changed 11 years ago by Jeroen Demeyer

Dependencies: #11130, #11891 #11890#11130, #11891, #11890
Description: modified (diff)

comment:5 Changed 11 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Status: newneeds_review

comment:6 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work
Work issues: doctests in sage/libs/pari

comment:7 Changed 11 years ago by Jeroen Demeyer

Milestone: sage-4.7.2sage-4.7.3
Status: needs_workneeds_review
Work issues: doctests in sage/libs/pari

comment:8 Changed 11 years ago by Jeroen Demeyer

Dependencies: #11130, #11891, #11890#11130, #11321, #11891, #11890, #11836

comment:9 Changed 11 years ago by William Stein

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 11 years ago by R. Andrew Ohana

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 11 years ago by Jeroen Demeyer

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 11 years ago by Jeroen Demeyer

@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 11 years ago by Jeroen Demeyer

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 11 years ago by Jeroen Demeyer

Milestone: sage-4.7.3

Milestone sage-4.7.3 deleted

comment:15 Changed 11 years ago by Jeroen Demeyer

Dependencies: #11130, #11321, #11891, #11890, #11836#11130, #11321, #11891, #11890, #11836, #11952
Milestone: sage-4.8

comment:16 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

Changed 11 years ago by Jeroen Demeyer

Attachment: 11904.patch added

comment:17 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:18 Changed 11 years ago by Jeroen Demeyer

Dependencies: #11130, #11321, #11891, #11890, #11836, #11952sage-4.8.alpha2

comment:19 Changed 11 years ago by John 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 11 years ago by John Cremona

Reviewers: John Cremona
Status: needs_reviewpositive_review

Happy Christmas!

comment:21 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-4.8.alpha5
Resolution: fixed
Status: positive_reviewclosed

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

Note: See TracTickets for help on using tickets.