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 )
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 8 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
- 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
- Dependencies changed from #11891 to #11130, #11891 #11890
comment:4 Changed 8 years ago by
- Dependencies changed from #11130, #11891 #11890 to #11130, #11891, #11890
- Description modified (diff)
comment:5 Changed 8 years ago by
- Status changed from new to needs_review
comment:6 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to doctests in sage/libs/pari
comment:7 Changed 8 years ago by
- 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
- Dependencies changed from #11130, #11891, #11890 to #11130, #11321, #11891, #11890, #11836
comment:9 follow-ups: ↓ 11 ↓ 12 ↓ 13 Changed 7 years ago by
comment:10 Changed 7 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 in reply to: ↑ 9 Changed 7 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 in reply to: ↑ 9 Changed 7 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 in reply to: ↑ 9 Changed 7 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 7 years ago by
- 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
- Description modified (diff)
Changed 7 years ago by
comment:17 Changed 7 years ago by
- Description modified (diff)
comment:18 Changed 7 years ago by
- Dependencies changed from #11130, #11321, #11891, #11890, #11836, #11952 to sage-4.8.alpha2
comment:19 Changed 7 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 7 years ago by
- Reviewers set to John Cremona
- Status changed from needs_review to positive_review
Happy Christmas!
comment:21 Changed 7 years ago by
- 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!
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.