Opened 6 years ago
Closed 6 years ago
#17033 closed defect (fixed)
Conversion of strings into function fields
Reported by: | SimonKing | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | algebra | Keywords: | |
Cc: | Merged in: | ||
Authors: | Simon King | Reviewers: | Ben Hutz |
Report Upstream: | N/A | Work issues: | |
Branch: | d0f6410 (Commits) | Commit: | d0f641076472507aeebf366bb9d5949a48c58e9a |
Dependencies: | Stopgaps: |
Description (last modified by )
As reported on sage-devel, the following used to fail, since conversion of strings to function fields was not implemented.
sage: S.<x, y> = K[] sage: I = S*[x^2 - y^2, y-t] sage: I.groebner_basis() [x^2 - t^2, y - t]
Change History (16)
comment:1 Changed 6 years ago by
- Branch set to u/SimonKing/function_field_conversion
comment:2 Changed 6 years ago by
- Commit set to a16bc05b502d6a35b113edcb4eb4221d630f9561
- Component changed from PLEASE CHANGE to algebra
- Description modified (diff)
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Type changed from PLEASE CHANGE to defect
comment:4 Changed 6 years ago by
The patchbot reports some numerical noise that is (I think) clearly not caused by the commit of this ticket. And on my laptop, all tests pass.
comment:5 Changed 6 years ago by
Would you expect something like the following to work:
K.<t>=FunctionField(QQ) R.<x>=PolynomialRing(QQ) M.<z> = K.extension(x^7-x-t); M('z')
It is giving the same str error that was seen before.
comment:6 follow-up: ↓ 7 Changed 6 years ago by
- Status changed from needs_review to needs_work
setting this to 'needs-work' so that it gets noticed.
comment:7 in reply to: ↑ 6 Changed 6 years ago by
- Status changed from needs_work to needs_review
Replying to bhutz:
setting this to 'needs-work' so that it gets noticed.
Why do you think 'needs work' will be more visible? Just on the contrary, a potential reviewer will not look at this ticket if it wrongly says 'needs work'.
comment:8 follow-up: ↓ 9 Changed 6 years ago by
err..I am the reviewer, I'm trying to get the attention of the author to respond to the question from 3 weeks ago. a 'needs-work' seems more likely to get the attention of the author.
comment:9 in reply to: ↑ 8 Changed 6 years ago by
- Status changed from needs_review to needs_work
Replying to bhutz:
err..I am the reviewer, I'm trying to get the attention of the author to respond to the question from 3 weeks ago. a 'needs-work' seems more likely to get the attention of the author.
Oops. It was "needs review", so, I thought it was indeed "needs review". But if it doesn't fix the problem, then "needs work" is appropriate...
comment:10 Changed 6 years ago by
Here is what the element constructor does:
if x.parent() is self._ring: return FunctionFieldElement_polymod(self, x) if isinstance(x, FunctionFieldElement): return FunctionFieldElement_polymod(self, self._ring(x.element())) return FunctionFieldElement_polymod(self, self._ring(x))
That's rather silly, since the last line does exactly what the first two lines do, if x.parent() is self._ring
(then, self._ring(x)
is supposed to return x
unaltered). I'll try if removing the first two lines does the job.
comment:11 Changed 6 years ago by
Surprisingly, it does not do the job.
Or not surprisingly. After all, the element constructor is only able to coerce things that live in self._ring
, but it is not able to deal with string representations of what it adds to self._ring
.
comment:12 Changed 6 years ago by
I think a straight forward way out is to use the correct variable name for internal representation.
With the commits that I am now testing, one would get
sage: K.<t> = FunctionField(QQ) sage: R.<x> = QQ[] sage: M.<z> = K.extension(x^7-x-t) sage: M(x) # I guess this is important for backwards compatibility z sage: M('z') z sage: M('x') Traceback (most recent call last): ... TypeError: unable to convert string
But before pushing, I need to run tests...
comment:13 Changed 6 years ago by
- Commit changed from a16bc05b502d6a35b113edcb4eb4221d630f9561 to d0f641076472507aeebf366bb9d5949a48c58e9a
comment:14 Changed 6 years ago by
- Status changed from needs_work to needs_review
Please test if the new commit fixes the remaining issues.
comment:15 Changed 6 years ago by
- Reviewers set to Ben Hutz
- Status changed from needs_review to positive_review
Yes that seems like a reasonable solution here and the remaining issues are fixed.
comment:16 Changed 6 years ago by
- Branch changed from u/SimonKing/function_field_conversion to d0f641076472507aeebf366bb9d5949a48c58e9a
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Fix conversion string -> function field