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 SimonKing)

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 SimonKing

  • Branch set to u/SimonKing/function_field_conversion

comment:2 Changed 6 years ago by SimonKing

  • Authors set to Simon King
  • Commit set to a16bc05b502d6a35b113edcb4eb4221d630f9561
  • Component changed from PLEASE CHANGE to algebra
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

a16bc05Fix conversion string -> function field

comment:3 Changed 6 years ago by SimonKing

  • Type changed from PLEASE CHANGE to defect

comment:4 Changed 6 years ago by SimonKing

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 bhutz

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: Changed 6 years ago by bhutz

  • 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 SimonKing

  • 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: Changed 6 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 6 years ago by SimonKing

  • 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 SimonKing

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 SimonKing

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 SimonKing

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 git

  • Commit changed from a16bc05b502d6a35b113edcb4eb4221d630f9561 to d0f641076472507aeebf366bb9d5949a48c58e9a

Branch pushed to git repo; I updated commit sha1. New commits:

10ae48fMerge branch 'develop' into t/17033/function_field_conversion
d0f6410Use the same generator name for function field extension and underlying polynomial ring

comment:14 Changed 6 years ago by SimonKing

  • 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 bhutz

  • 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 vbraun

  • Branch changed from u/SimonKing/function_field_conversion to d0f641076472507aeebf366bb9d5949a48c58e9a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.