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: |
Description (last modified by )
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)
Change History (22)
comment:1 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 11 years ago by
Summary: | Change default variable name for NumberFieldElement -> PARI conversion → Change default variable name for NumberField and NumberFieldElement -> PARI conversion |
---|
comment:3 Changed 11 years ago by
Dependencies: | #11891 → #11130, #11891 #11890 |
---|
comment:4 Changed 11 years ago by
Dependencies: | #11130, #11891 #11890 → #11130, #11891, #11890 |
---|---|
Description: | modified (diff) |
comment:5 Changed 11 years ago by
Authors: | → Jeroen Demeyer |
---|---|
Status: | new → needs_review |
comment:6 Changed 11 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → doctests in sage/libs/pari |
comment:7 Changed 11 years ago by
Milestone: | sage-4.7.2 → sage-4.7.3 |
---|---|
Status: | needs_work → needs_review |
Work issues: | doctests in sage/libs/pari |
comment:8 Changed 11 years ago by
Dependencies: | #11130, #11891, #11890 → #11130, #11321, #11891, #11890, #11836 |
---|
comment:9 follow-ups: 11 12 13 Changed 11 years ago by
comment:10 Changed 11 years ago by
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 Changed 11 years ago by
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 Changed 11 years ago by
@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 Changed 11 years ago by
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:15 Changed 11 years ago by
Dependencies: | #11130, #11321, #11891, #11890, #11836 → #11130, #11321, #11891, #11890, #11836, #11952 |
---|---|
Milestone: | → sage-4.8 |
comment:16 Changed 11 years ago by
Description: | modified (diff) |
---|
Changed 11 years ago by
Attachment: | 11904.patch added |
---|
comment:17 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:18 Changed 11 years ago by
Dependencies: | #11130, #11321, #11891, #11890, #11836, #11952 → sage-4.8.alpha2 |
---|
comment:19 Changed 11 years ago by
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
Reviewers: | → John Cremona |
---|---|
Status: | needs_review → positive_review |
Happy Christmas!
comment:21 Changed 11 years ago by
Merged in: | → sage-4.8.alpha5 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Happy Christmas to you too, and see you on monday!
Quick question? What happens if I do:
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.