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

Laurent power series fail unique representation #24420

Closed
videlec opened this issue Dec 22, 2017 · 28 comments
Closed

Laurent power series fail unique representation #24420

videlec opened this issue Dec 22, 2017 · 28 comments

Comments

@videlec
Copy link
Contributor

videlec commented Dec 22, 2017

sage: L.<q> = LaurentSeriesRing(QQ)
sage: LaurentSeriesRing(QQ, 'q') is L
False

to be compared with

sage: R.<q> = PolynomialRing(QQ)
sage: PolynomialRing(QQ, 'q') is R
True

We also fix

sage: LaurentSeriesRing(Zmod(4), 'x').category()
Category of fields

We also remove the classes LaurentSeriesRing_generic, LaurentSeriesRing_domain, LaurentSeriesRing_field in favor of a unique LaurentSeriesRing.

Component: algebra

Author: Vincent Delecroix

Branch/Commit: bcf577d

Reviewer: Travis Scrimshaw

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

@videlec videlec added this to the sage-8.2 milestone Dec 22, 2017
@videlec

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor Author

videlec commented Dec 22, 2017

Author: Vincent Delecroix

@videlec
Copy link
Contributor Author

videlec commented Dec 26, 2017

New commits:

f779b1024420: clean laurent series

@videlec
Copy link
Contributor Author

videlec commented Dec 26, 2017

Branch: u/vdelecroix/24420

@videlec
Copy link
Contributor Author

videlec commented Dec 26, 2017

Commit: f779b10

@videlec

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 26, 2017

Changed commit from f779b10 to 1adb84c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 26, 2017

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1adb84c24420: clean laurent series

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2017

Changed commit from 1adb84c to cdf836a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2017

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cdf836a24420: clean laurent series

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

34769e124420: clean laurent series

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2018

Changed commit from cdf836a to 34769e1

@videlec
Copy link
Contributor Author

videlec commented Jan 2, 2018

comment:8

rebased on 8.2.beta2

@tscrim
Copy link
Collaborator

tscrim commented Jan 2, 2018

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jan 2, 2018

comment:9

Since you are going to do the category imports in the __init__ (which I'm not 100% in agreement with), I would make it in as small as scope as possible:

         from sage.categories.rings import Rings
         from sage.categories.integral_domains import IntegralDomains
         from sage.categories.fields import Fields
-        from sage.categories.complete_discrete_valuation import (
-                CompleteDiscreteValuationRings, CompleteDiscreteValuationFields)
 
         base_ring = power_series.base_ring()
         if base_ring in Fields():
+            from sage.categories.complete_discrete_valuation import CompleteDiscreteValuationFields
             category = CompleteDiscreteValuationFields()
         elif base_ring in IntegralDomains():
             category = IntegralDomains()
         elif base_ring in Rings().Commutative():
             category = Rings().Commutative()
         else:
             raise ValueError('unrecognized base ring')

Also, don't we want to use names=power_series.variable_names() in case we (eventually support) have multiple variable names. Otherwise LGTM.

@videlec
Copy link
Contributor Author

videlec commented Jan 2, 2018

comment:10

Replying to @tscrim:

Since you are going to do the category imports in the __init__ (which I'm not 100% in agreement with)

Why? I am happy to move them at the module scope if it is a better practice.

@tscrim
Copy link
Collaborator

tscrim commented Jan 2, 2018

comment:11

Replying to @videlec:

Replying to @tscrim:

Since you are going to do the category imports in the __init__ (which I'm not 100% in agreement with)

Why? I am happy to move them at the module scope if it is a better practice.

It slows down the construction of the ring.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2018

Changed commit from 34769e1 to fd34a95

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2018

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

fd34a9524420: move imports / name -> names

@videlec
Copy link
Contributor Author

videlec commented Jan 2, 2018

comment:13

done

@tscrim
Copy link
Collaborator

tscrim commented Jan 2, 2018

comment:14

Thanks. One last little thing: you don't use CompleteDiscreteValuationRings.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bcf577d24420: move imports / name -> names

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 2, 2018

Changed commit from fd34a95 to bcf577d

@videlec
Copy link
Contributor Author

videlec commented Jan 2, 2018

comment:16

Indeed!

@tscrim
Copy link
Collaborator

tscrim commented Jan 2, 2018

comment:17

Thanks.

@vbraun
Copy link
Member

vbraun commented Jan 5, 2018

Changed branch from u/vdelecroix/24420 to bcf577d

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

3 participants