Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fraction field of Ore polynomial ring #29678

Closed
xcaruso opened this issue May 12, 2020 · 60 comments
Closed

Fraction field of Ore polynomial ring #29678

xcaruso opened this issue May 12, 2020 · 60 comments

Comments

@xcaruso
Copy link
Contributor

xcaruso commented May 12, 2020

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.

Depends on #29629

Component: algebra

Author: Xavier Caruso

Branch/Commit: c40eebe

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/29678

@xcaruso xcaruso added this to the sage-9.1 milestone May 12, 2020
@xcaruso
Copy link
Contributor Author

xcaruso commented May 12, 2020

Branch: u/caruso/ore_functions

@xcaruso
Copy link
Contributor Author

xcaruso commented May 12, 2020

Last 10 new commits:

52932e0fix typos
31a3c07add documentation
0a232cdLaTeX representation of derivations
d456551implement Hilbert shift
f225cbcfix pyflakes
9067b73Ore functions (first draft)
d0a9712Hilbert shift
650079dpolish implementation
f6835a3implement extend_to_fraction_field for derivations
2e9458bregister coercion from a Ore ring to its fraction field

@xcaruso
Copy link
Contributor Author

xcaruso commented May 12, 2020

Author: Xavier Caruso

@xcaruso
Copy link
Contributor Author

xcaruso commented May 12, 2020

Commit: 2e9458b

@xcaruso
Copy link
Contributor Author

xcaruso commented May 12, 2020

Changed dependencies from #29269 to #29629

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

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

a74ddacreduced trace and reduced norm

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

Changed commit from 2e9458b to a74ddac

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2020

Changed commit from a74ddac to f50aca6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2020

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

7cf9e02doctest in ore_function_field.py
f50aca6more doctests

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 17, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

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

0c8f04dMerge branch 'develop' into t/29678/ore_functions
b35e1d0fix one doctest
131d78eMerge branch 'develop' into t/29629/ore_polynomials_rebased
4f273e8Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions
648cf6dremove trailing spaces

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

Changed commit from f50aca6 to 648cf6d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 25, 2020

Changed commit from 648cf6d to 15edb98

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 25, 2020

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

15edb98fix pickling

@xcaruso
Copy link
Contributor Author

xcaruso commented Jun 25, 2020

comment:8

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?

@tscrim
Copy link
Collaborator

tscrim commented Jun 26, 2020

comment:9

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 5, 2020

Changed commit from 15edb98 to 070792b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 5, 2020

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

445913aabstract method degree raises NotImplementedError
fdbded7add full-stop after math formulas
c778356remove import sage
bf17a36Merge branch 'develop' of https://github.com/sagemath/sage into t/29629/ore_polynomials_rebased
f7b0eddMerge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions
94a1a5clazy import
15f6dfafix indentation
a9bae69update documentation
3cf021aMerge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions
070792blet extend_to_fraction_field be more tolerant

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 5, 2020

comment:11

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

@tscrim
Copy link
Collaborator

tscrim commented Jul 6, 2020

comment:12

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

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

c7d0acdreorganize documentation
ec0e24bElement -> element_class
8c0efe7add a doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

Changed commit from 070792b to 8c0efe7

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 6, 2020

comment:14

Replying to @tscrim:

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

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

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

44c9913typos

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 9, 2020

comment:23

Replying to @tscrim:

I think you also need the conf.py file.

Oh, I see. Thanks.

@tscrim
Copy link
Collaborator

tscrim commented Jul 9, 2020

comment:24

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?

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 10, 2020

comment:25

My conf.py is a symlink, isn't it?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

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

d3c744eaccept more errors in trying to inverse the twisting morphism

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

Changed commit from 548a1dc to d3c744e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

Changed commit from d3c744e to 7e3abda

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

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

7e3abdatypos in doctests

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2020

comment:28

Ah, I see. Git is just handling it in a strange way. Sorry for the noise.

However, no bare excepts.

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 10, 2020

comment:29

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?

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2020

comment:30

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.

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 10, 2020

comment:31

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.

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2020

comment:32

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

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2020

Changed commit from 7e3abda to 1c7a67c

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2020

comment:33

Here is a version with my docbuild fixes from #29629.


New commits:

f111d30Merge branch 'u/caruso/ore_polynomials_rebased' of git://trac.sagemath.org/sage into u/caruso/ore_polynomials_rebased
5d822f3Fixing the doc and != for polynomial injection maps.
1c7a67cMerge branch 'u/tscrim/ore_polynomials-29629' of git://trac.sagemath.org/sage into u/caruso/ore_polynomials-29629

@tscrim
Copy link
Collaborator

tscrim commented Jul 10, 2020

Changed branch from u/caruso/ore_functions to u/tscrim/ore_functions-29678

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 11, 2020

Changed branch from u/tscrim/ore_functions-29678 to u/caruso/ore_functions-29678

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 11, 2020

Changed commit from 1c7a67c to b327f51

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 11, 2020

comment:35

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:

8c0efe7add a doctest
44c9913typos
e1fc660more doctests
59d3ed2Merge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions
f6ea4fdMerge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions
548a1dcadd conf.py
d3c744eaccept more errors in trying to inverse the twisting morphism
7e3abdatypos in doctests
055bdebMerge branch 't/29629/ore_polynomials_rebased' into t/29678/ore_functions
b327f51add list of exceptions

@tscrim
Copy link
Collaborator

tscrim commented Jul 11, 2020

comment:36

Whoops, bad copy-paste of the branch name. ^^;; If the patchbot comes back green, then positive review. Thank you.

@xcaruso
Copy link
Contributor Author

xcaruso commented Jul 11, 2020

comment:37

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?

@tscrim
Copy link
Collaborator

tscrim commented Jul 12, 2020

comment:38

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2020

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

c40eebe:module: -> :mod:

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2020

Changed commit from b327f51 to c40eebe

@tscrim
Copy link
Collaborator

tscrim commented Jul 12, 2020

comment:40

Thank you. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Jul 12, 2020

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Jul 19, 2020

Changed branch from u/caruso/ore_functions-29678 to c40eebe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants