Opened 19 months ago
Closed 16 months ago
#28862 closed enhancement (fixed)
Localization of integral domains
Reported by:  soehms  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  commutative algebra  Keywords:  rings integral domains localization fraction field 
Cc:  tscrim, vdelecroix  Merged in:  
Authors:  Sebastian Oehms  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  44f816d (Commits, GitHub, GitLab)  Commit:  44f816ddf5aee524df452434b9e6ffcee72a17bf 
Dependencies:  Stopgaps: 
Description
Localization is an important ring construction tool, which isn't available in Sage, so far. Whenever you have to extend a given integral domain such that it contains the inverses of a finite set of elements but should allow non injective homomorphic images this construction will be needed. Such situations occur for base rings of certain algebras, for example Hecke algebras as pointed out in comment 9 of ticket #27371.
This ticket implements a class for this construction following an example given in the reference pages on coercion (thanks to Vincent for this hint): coercion example
Change History (15)
comment:1 Changed 19 months ago by
 Branch set to u/soehms/localization
comment:2 Changed 19 months ago by
 Commit set to 8065d4d01dbad333b0726084f35815281891b67c
 Status changed from new to needs_review
comment:3 Changed 19 months ago by
 Commit changed from 8065d4d01dbad333b0726084f35815281891b67c to 96da2b04862bfac29b4fd777e03f1a63eb3f1450
Branch pushed to git repo; I updated commit sha1. New commits:
96da2b0  28862: fix pyflakes hints

comment:4 Changed 19 months ago by
 Commit changed from 96da2b04862bfac29b4fd777e03f1a63eb3f1450 to 804a62cea35badee839342e57a139742a28a06a4
Branch pushed to git repo; I updated commit sha1. New commits:
804a62c  28862: doctest syntax

comment:5 Changed 19 months ago by
 Commit changed from 804a62cea35badee839342e57a139742a28a06a4 to 35e87fcbf81e656137fd13983a9ce3630364e405
Branch pushed to git repo; I updated commit sha1. New commits:
35e87fc  28862: typos and some cleaning

comment:6 Changed 17 months ago by
 Commit changed from 35e87fcbf81e656137fd13983a9ce3630364e405 to e362c57433b31b26f8d3c1e31e580381ee3b245c
Branch pushed to git repo; I updated commit sha1. New commits:
e362c57  Merge branch 'u/soehms/localization' of git://trac.sagemath.org/sage into localization_28862

comment:7 Changed 17 months ago by
Just fixed merge conflict!
comment:8 Changed 17 months ago by
Overall this looks good and seems like a feature that should have been in Sage sometime ago. However, I do have some comments:
Coercion from Localization:: +Coercion from a localization::
I am not sure I like this:
parent.__make_element_class__(FractionFieldEmbedding)
It doesn't feel right to be getting into the internal workings. Homsets are also somewhat special (and we don't have a good mechanism for parents with multiple element classes), so I use one of the following:
parent.element_class(foo) FractionFieldEmbedding(foo)
I would get rid of this comment:
The implementation of this class is based on an example given in the reference pages on coercion: `coercion example <http://doc.sagemath.org/html/en/reference/coercion/index.html?highlight=localization#example>`_
and instead just say localizations inherit from IntegralDomain
and the consequences. I might also consider moving much of the modulelevel documentation to the classlevel documentation as it is something specific to that implementation and is easier to see from the command line via ?
.
I don't think you need to say where the example comes from. So I would remove this:
taken from comment #9 of :trac:`27371`
The items of INPUT:
do not end with a period/fullstop. The OUTPUT:
should either start with a capital letter or not end with a period/fullstop.
The list
is redundant here:
return sorted(list(set(add_units_result))) +return sorted(set(add_units_result))
For __init__
, a good thing to put in there is a TestSuite(foo).run()
call.
Realizes
> Realize
(although I would say Compute
probably).
is_unit
and inverse_of_unit
should have at least a oneline description.
In _richcmp_
, this test is vacuous:
if self.parent() != other.parent(): return super(Localization, self)._richcmp_(other, op)
since by the time it gets there, it must be in the same parent. Also
INPUT (to the constructor): +INPUT:
Error messages should start with a lowercase letter.
 Return the ith generator of self which is  the ith generator of the base ring. + Return the ``i``th generator of ``self``, which is + the ``i``th generator of the base ring.
comment:9 Changed 17 months ago by
 Commit changed from e362c57433b31b26f8d3c1e31e580381ee3b245c to 0cbd98533749bf2e71c7ef7aecf8a89868060b62
comment:10 Changed 17 months ago by
I could follow all of your suggestions except the one about the definition of the coercion into the field of fraction.
Maybe I didn't get your suggestion, correctly. But, if I try it like this:
+ return FractionFieldEmbedding(S, self, category=parent.homset_category())  return parent.__make_element_class__(FractionFieldEmbedding)(S, self, category=parent.homset_category())
I'm getting the following:
sage: R.<x> = QQ[] sage: L = R.localization(x**2+1) sage: f = L.fraction_field().coerce_map_from(L) sage: TestSuite(f).run() Failure in _test_category: Traceback (most recent call last): ... The following tests failed: _test_category
Can we ignore that? An according doctest has been in the code with respect to the coercion from the base ring into its field of fraction.
comment:11 Changed 17 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Ah, so this pattern has been used in many other places in the Sage code. Thus it is good to do this. I might need to make some appropriate changes to some of my code now that I know about this pattern. Thanks.
comment:12 Changed 17 months ago by
 Status changed from positive_review to needs_review
Ah, one more quick thing: the long (>80 chars) lines in the EXAMPLES::
of the modulelevel doc of localization.py
. I thought I had that in my comments. For the code part, it is less important, but you can also have outputs like:
Multivariate Polynomial Ring in u0, u1, u2, q over Integer Ring localized at (q, q + 1, u2, u1, u1  u2, u0, u0  u2, u0  u1, u2*q  u1, u2*q  u0, u1*q  u2, u1*q  u0, u0*q  u2, u0*q  u1)
In particular, allowing the code parts to break the 80 char/line guideline is okay when it makes the break more natural IMO. Although it is still good to try and follow it.
Once changed, you can set it back to a positive review.
comment:13 Changed 17 months ago by
 Commit changed from 0cbd98533749bf2e71c7ef7aecf8a89868060b62 to 44f816ddf5aee524df452434b9e6ffcee72a17bf
Branch pushed to git repo; I updated commit sha1. New commits:
44f816d  28862: docstring formatting

comment:14 Changed 17 months ago by
 Status changed from needs_review to positive_review
Now, PDFdocs look much nicer! Thanks a lot!
comment:15 Changed 16 months ago by
 Branch changed from u/soehms/localization to 44f816ddf5aee524df452434b9e6ffcee72a17bf
 Resolution set to fixed
 Status changed from positive_review to closed
The first example I've included in my initial attempt raises the following questions:
How should the behavior of matrix inversion over a localization be? By default it is a matrix over the field of fraction. In the docstring of
sage.matrix.matrix0.__invert__
this is explained by analogy to the behavior over the integers:parent(~1) == Rational Field
. I think for a matrix over a localization the user would expect to obtain a matrix over an appropriate localization again (being equal or extending the former one). Shouldn't we change this behavior, as well?New commits:
28862: initial version