Opened 21 months ago
Closed 19 months ago
#29678 closed enhancement (fixed)
Fraction field of Ore polynomial ring
Reported by:  caruso  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  algebra  Keywords:  
Cc:  Merged in:  
Authors:  Xavier Caruso  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  c40eebe (Commits, GitHub, GitLab)  Commit:  c40eebead49dba2a9b37a3c1897a7eb71688296e 
Dependencies:  #29629  Stopgaps: 
Description
We implement the fraction field of Ore polynomial rings.
The elements of the fraction field are represented by fractions of the form denom^(1) * num
.
Change History (41)
comment:1 Changed 21 months ago by
 Branch set to u/caruso/ore_functions
comment:2 Changed 21 months ago by
 Commit set to 2e9458baa421e1aba7d56fa3fe9eb8a5ff2d411e
 Dependencies changed from #29269 to #29629
comment:3 Changed 21 months ago by
 Commit changed from 2e9458baa421e1aba7d56fa3fe9eb8a5ff2d411e to a74ddac387d89650325b92ef53237343fb7b2ac9
Branch pushed to git repo; I updated commit sha1. New commits:
a74ddac  reduced trace and reduced norm

comment:4 Changed 21 months ago by
 Commit changed from a74ddac387d89650325b92ef53237343fb7b2ac9 to f50aca6f0d42e964521c5b327848ec33f8896c36
comment:5 Changed 21 months ago by
 Milestone changed from sage9.1 to sage9.2
comment:6 Changed 20 months ago by
 Commit changed from f50aca6f0d42e964521c5b327848ec33f8896c36 to 648cf6d95830c5ae4279415019ce73ef4bd6758d
Branch pushed to git repo; I updated commit sha1. New commits:
0c8f04d  Merge branch 'develop' into t/29678/ore_functions

b35e1d0  fix one doctest

131d78e  Merge branch 'develop' into t/29629/ore_polynomials_rebased

4f273e8  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

648cf6d  remove trailing spaces

comment:7 Changed 19 months ago by
 Commit changed from 648cf6d95830c5ae4279415019ce73ef4bd6758d to 15edb980ec75201977aebfd74afdee1a04497ba7
Branch pushed to git repo; I updated commit sha1. New commits:
15edb98  fix pickling

comment:8 Changed 19 months ago by
 Status changed from new to needs_review
There is still a small issue with category (elements are not instances of parent.category().element_class
) but I don't know how I should fix it. Any ideas?
comment:9 Changed 19 months ago by
Typically that means you are returning elements that are not instances of ParentObject.element_class
. If this is for a morphism parent, you can simply silence that warning. (This is a common practice as there can be many morphism types, but we don't (yet) have a good framework for these things. I have seen a workaround for this that just dynamically creates the new element class, but I forget where in the Sage code this is done.)
comment:10 Changed 19 months ago by
 Commit changed from 15edb980ec75201977aebfd74afdee1a04497ba7 to 070792b9c20e7e9f0c7f2aaac0989dd8b6c53c7f
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
445913a  abstract method degree raises NotImplementedError

fdbded7  add fullstop after math formulas

c778356  remove import sage

bf17a36  Merge branch 'develop' of https://github.com/sagemath/sage into t/29629/ore_polynomials_rebased

f7b0edd  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

94a1a5c  lazy import

15f6dfa  fix indentation

a9bae69  update documentation

3cf021a  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

070792b  let extend_to_fraction_field be more tolerant

comment:11 Changed 19 months ago by
Here is my issue:
sage: k.<a> = GF(5^3) sage: Frob = k.frobenius_endomorphism() sage: S.<x> = k['x', Frob] sage: K = S.fraction_field() sage: K(x)._test_category() Traceback (most recent call last): ... AssertionError: False is not true
K.category().element_class
is sage.categories.algebras.Algebras.element_class
but K(x)
is an instance is an instance of sage.rings.polynomial.ore_function_element.OreFunction_with_large_center
which, apparently, does not define from the former (although OreFunction_with_large_center
derives from AlgebraElement
).
comment:12 followup: ↓ 14 Changed 19 months ago by
This should be your problem: In OreFunctionField._element_constructor_
, you need to change
self.Element(self, *args, **kwds) +self.element_class(self, *args, **kwds)
This also needs to be changed for OrePolynomialRing._element_constructor_
:
C = self.Element +C = self.element_class
One other thing: the extend_to_fraction_field
needs doctests.
comment:13 Changed 19 months ago by
 Commit changed from 070792b9c20e7e9f0c7f2aaac0989dd8b6c53c7f to 8c0efe704d0dee6efb6267b45150fd0bd7d6cea0
comment:14 in reply to: ↑ 12 ; followup: ↓ 16 Changed 19 months ago by
Replying to tscrim:
This should be your problem: In
OreFunctionField._element_constructor_
, you need to changeself.Element(self, *args, **kwds) +self.element_class(self, *args, **kwds)This also needs to be changed for
OrePolynomialRing._element_constructor_
:C = self.Element +C = self.element_class
Thanks, It indeed fixes the problem!
One other thing: the
extend_to_fraction_field
needs doctests.
There were already doctests. But I've added one more.
comment:15 Changed 19 months ago by
 Commit changed from 8c0efe704d0dee6efb6267b45150fd0bd7d6cea0 to 44c9913ab197d0c13f7d24a055c987d0257c5f31
Branch pushed to git repo; I updated commit sha1. New commits:
44c9913  typos

comment:16 in reply to: ↑ 14 Changed 19 months ago by
comment:17 Changed 19 months ago by
Sorry, two places: RingDerivationWithTwist_generic.extend_to_fraction_field
as well.
comment:18 Changed 19 months ago by
 Commit changed from 44c9913ab197d0c13f7d24a055c987d0257c5f31 to e1fc6605e40127638cbe28bd130e7c8abb73cbf7
Branch pushed to git repo; I updated commit sha1. New commits:
e1fc660  more doctests

comment:19 Changed 19 months ago by
Fixed.
Also, I tried to reorganize a bit the documentation and separate commutative polynomials from noncommutative polynomials. I created a new file doc/en/reference/noncommutative_polynomial_rings/index.rst
but I probably did something wrong because this file is not found when building the documentation. Do you know what's going on, here?
comment:20 followup: ↓ 23 Changed 19 months ago by
I think you also need the conf.py
file.
comment:21 Changed 19 months ago by
 Commit changed from e1fc6605e40127638cbe28bd130e7c8abb73cbf7 to f6ea4fdfb95860f21606bf80fad91d1bc1600a43
Branch pushed to git repo; I updated commit sha1. New commits:
1cc0ed8  Merge branch 'develop' into t/29629/ore_polynomials_rebased

59d3ed2  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

200d7bd  update doctests

f6ea4fd  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

comment:22 Changed 19 months ago by
 Commit changed from f6ea4fdfb95860f21606bf80fad91d1bc1600a43 to 548a1dcdd1e70a703a7a2ec06f5f326519e7a92b
Branch pushed to git repo; I updated commit sha1. New commits:
548a1dc  add conf.py

comment:23 in reply to: ↑ 20 Changed 19 months ago by
comment:24 Changed 19 months ago by
Rather than be a direct file that refers back to conf_sub.py
, the other folders are symlinks. Could you do the same thing here?
comment:25 Changed 19 months ago by
My conf.py
is a symlink, isn't it?
comment:26 Changed 19 months ago by
 Commit changed from 548a1dcdd1e70a703a7a2ec06f5f326519e7a92b to d3c744ee08178f1b541cc2c6b865b83a106f72cd
Branch pushed to git repo; I updated commit sha1. New commits:
d3c744e  accept more errors in trying to inverse the twisting morphism

comment:27 Changed 19 months ago by
 Commit changed from d3c744ee08178f1b541cc2c6b865b83a106f72cd to 7e3abda2c626de0a3370d4f8bec2c00f39cb490c
Branch pushed to git repo; I updated commit sha1. New commits:
7e3abda  typos in doctests

comment:28 Changed 19 months ago by
Ah, I see. Git is just handling it in a strange way. Sorry for the noise.
However, no bare excepts.
comment:29 Changed 19 months ago by
Btw, I saw that there are pyflakes warnings in conf.py
due to the imports (on two consecutive lines):
from sage.docs.conf import release, exclude_patterns from sage.docs.conf import *
Shouldn't we remove the second import?
comment:30 followup: ↓ 31 Changed 19 months ago by
I wouldn't touch the conf.py
files here. That should be a separate ticket as that is outside of the scope and could have larger impact to Sage's doc.
comment:31 in reply to: ↑ 30 Changed 19 months ago by
Replying to tscrim:
I wouldn't touch the
conf.py
files here. That should be a separate ticket as that is outside of the scope and could have larger impact to Sage's doc.
Yes, I agree.
comment:32 Changed 19 months ago by
The only thing missing from me giving this a positive review is taking care of the bare except:
(this can catch keyboard interrupts I've been told, so it is essentially prohibited to have in Sage code).
comment:33 Changed 19 months ago by
 Branch changed from u/caruso/ore_functions to u/tscrim/ore_functions29678
 Commit changed from 7e3abda2c626de0a3370d4f8bec2c00f39cb490c to 1c7a67c8638b3984256281b5da382daf69ec36ae
Here is a version with my docbuild fixes from #29629.
New commits:
f111d30  Merge branch 'u/caruso/ore_polynomials_rebased' of git://trac.sagemath.org/sage into u/caruso/ore_polynomials_rebased

5d822f3  Fixing the doc and != for polynomial injection maps.

1c7a67c  Merge branch 'u/tscrim/ore_polynomials29629' of git://trac.sagemath.org/sage into u/caruso/ore_polynomials29629

comment:34 Changed 19 months ago by
 Branch changed from u/tscrim/ore_functions29678 to u/caruso/ore_functions29678
comment:35 Changed 19 months ago by
 Commit changed from 1c7a67c8638b3984256281b5da382daf69ec36ae to b327f519bd14c3086372bbf8dbd4c89b60b191ea
OK, I've added an explicit list of exceptions.
I also rebased the commit on my branch because I thought you have forgotten to include the core of this ticket in your merge :). I hope everything is now merged correctly.
Last 10 new commits:
8c0efe7  add a doctest

44c9913  typos

e1fc660  more doctests

59d3ed2  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

f6ea4fd  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

548a1dc  add conf.py

d3c744e  accept more errors in trying to inverse the twisting morphism

7e3abda  typos in doctests

055bdeb  Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions

b327f51  add list of exceptions

comment:36 Changed 19 months ago by
Whoops, bad copypaste of the branch name. ^^;;
If the patchbot comes back green, then positive review. Thank you.
comment:37 Changed 19 months ago by
The patchbots reported a bunch of errors in the documentation (some files are not found). However, I'm not sure that they are related to this ticket. Do you know what's happening here?
comment:38 Changed 19 months ago by
In ore_polynomial_ring.py
:
:module:`sage.rings.polynomial.ore_function_field` +:mod:`sage.rings.polynomial.ore_function_field`
That is at least the current docbuild failure.
comment:39 Changed 19 months ago by
 Commit changed from b327f519bd14c3086372bbf8dbd4c89b60b191ea to c40eebead49dba2a9b37a3c1897a7eb71688296e
Branch pushed to git repo; I updated commit sha1. New commits:
c40eebe  :module: > :mod:

comment:40 Changed 19 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thank you. LGTM.
comment:41 Changed 19 months ago by
 Branch changed from u/caruso/ore_functions29678 to c40eebead49dba2a9b37a3c1897a7eb71688296e
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
fix typos
add documentation
LaTeX representation of derivations
implement Hilbert shift
fix pyflakes
Ore functions (first draft)
Hilbert shift
polish implementation
implement extend_to_fraction_field for derivations
register coercion from a Ore ring to its fraction field