Opened 12 years ago
Last modified 10 years ago
#9054 closed enhancement
create a class for basic function_field arithmetic for Sage — at Version 48
Reported by: | was | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | algebra | Keywords: | |
Cc: | burcin, khwilson, mderickx, mstreng, novoselt, pbruin, minz, saraedum | Merged in: | |
Authors: | William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #9094, #11751 | Stopgaps: |
Description (last modified by )
One of the first things we learned at Sage Days 21: Function Fields, is that it is not even possible to really define or even do arithmetic in function fields *at all* in Sage! It's amazing that this most basic arithmetic still isn't supported, but it isn't (maybe it used to be via generic machinery, but got broken...?). The point of this ticket is to create classes for standard function field structures, along with support for arithmetic. This should be organized in a way similar to number fields.
For this code, the main point is to establish an API that works solidly. It will be insanely slow. A subsequent patch will make things fast.
See also: #9069, #9051, #9094, #9095.
Apply trac_9054-all-parts.patch, trac_9054_polynomial_base_field.patch,
trac_9054_zero.patch, trac_9054_codomain.patch,
trac_9054_doctest-2.patch, trac_9054-review.patch
Depends on #9094, #11751
Change History (67)
Changed 12 years ago by
comment:1 Changed 12 years ago by
- Cc burcin added
Changed 12 years ago by
Changed 12 years ago by
comment:2 follow-up: ↓ 5 Changed 12 years ago by
Changed 12 years ago by
Changed 12 years ago by
comment:3 Changed 12 years ago by
Looks like you forgot to add the file function_field_order
, so I wasn't able to doctest on top of your latest patch (let alone debug it). See also #9051 for added speed in the positive characteristic case.
Changed 12 years ago by
comment:4 Changed 12 years ago by
FunctionField? constructor clips names
sage: F = FunctionField(GF(7), 'bit') sage: F.gen() b
comment:5 in reply to: ↑ 2 Changed 12 years ago by
Replying to salmanhb:
There seems to be an issue with returning the base ring of a RationalFunctionField?. Neither base() nor base_ring() return the correct ring:
sage: K.<t> = FunctionField(QQ); K Rational function field in t over Rational Field sage: R1 = K.base(); R1 Rational function field in t over Rational Field sage: R2 = K.base_ring(); R2 Rational function field in t over Rational Field sage: R3.<s> = QQ[]; K3 = Frac(R3); K3 Fraction Field of Univariate Polynomial Ring in s over Rational Field sage: R3 Univariate Polynomial Ring in s over Rational Field sage: K3.base() == R3 True
The above is correct. To get what you want, use the constant_field() method.
sage: K.<t> = FunctionField(QQ); sage: K.constant_field() Rational Field
Changed 12 years ago by
comment:6 Changed 12 years ago by
Should be some automatic way to do the following:
K.<T> = FunctionField(GF(17)) P = T-5 f = P^5 R = K._ring R(f.element()).valuation(R(p.element()))
comment:7 Changed 12 years ago by
- Cc khwilson added
Changed 12 years ago by
flattened patch that incorporates all of patches 1-12 above into a single patch.
comment:8 Changed 12 years ago by
- Owner changed from AlexGhitza to was
Here is a link to the result of doctesting sage-4.4.4 + patches 1-12:
http://sage.math.washington.edu/home/wstein/patches/9054-part1-12.doctest.txt
The failed tests:
The following tests failed: sage -t devel/sage-main/sage/matrix/matrix2.pyx # 1 doctests failed sage -t devel/sage-main/sage/plot/matrix_plot.py # 0 doctests failed sage -t devel/sage-main/sage/modular/abvar/morphism.py # 1 doctests failed sage -t devel/sage-main/sage/modular/abvar/finite_subgroup.py # 1 doctests failed sage -t devel/sage-main/sage/tests/startup.py # 1 doctests failed sage -t devel/sage-main/sage/modular/modform/hecke_operator_on_qexp.py # 1 doctests failed sage -t devel/sage-main/sage/categories/function_fields.py # 5 doctests failed sage -t devel/sage-main/sage/rings/function_field/function_field_element.pyx # 14 doctests failed
comment:9 Changed 12 years ago by
- Cc mderickx added
comment:10 Changed 12 years ago by
- Cc mstreng added
comment:11 Changed 12 years ago by
- Cc pbruin added
comment:12 Changed 12 years ago by
- Description modified (diff)
comment:13 Changed 12 years ago by
NOTE: #9094 implements sqrt for polynomials, which is relevant to trac_9054-doctest.patch
comment:14 Changed 12 years ago by
I guess the doctest patch isn't really usefull addition to sage (although making it was a usefull learning experience for Peter Bruin and me since it was our first patch). The patch fixes some bugs which are also fixed in other patches in trac. Some are indeed fixed by #9094 (although i think this can be done faster and more elegant) and another one related calculating the valuation in fraction fields is fixed by 9051-FpT_4.patch in #9051.
Since I'm quite new to developing and using trac and hg etc. I would like to know what is the best thing to do now. And especially how to deal with the relating patches wich also contain fixes for stuff happening here.
comment:15 Changed 12 years ago by
- Description modified (diff)
comment:16 Changed 12 years ago by
Added an attachment that fixes all but three doctest failures. The remaining failures are:
sage -t "devel/sage-mderickx/sage/modular/abvar/morphism.py" # 1 failure sage -t "devel/sage-mderickx/sage/modular/abvar/finite_subgroup.py" # 1 failure sage -t "devel/sage-main/sage/modular/modform/hecke_operator_on_qexp.py" # 1 failure }}}They are all related since their error messages all end in:{{{ File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4700, in _echelonized_basis d = self._denominator(basis) File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4810, in _denominator d = d.lcm(x.denominator()) !AttributeError: 'int' object has no attribute 'lcm'}}}It would be nice if someone who has a better understanding of sage to fix this final bug, since then we would have no doctests failing anymore for this patch.
comment:17 Changed 12 years ago by
Oeps, wrong fromatting. Now a bit more readable:
sage -t "devel/sage-mderickx/sage/modular/abvar/morphism.py" # 1 failure sage -t "devel/sage-mderickx/sage/modular/abvar/finite_subgroup.py" # 1 failure sage -t "devel/sage-main/sage/modular/modform/hecke_operator_on_qexp.py" # 1 failure
They are all related since their error messages all end in:
File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4700, in _echelonized_basis d = self._denominator(basis) File "/Applications/sage/local/lib/python/site-packages/sage/modules/free_module.py", line 4810, in _denominator d = d.lcm(x.denominator()) AttributeError: 'int' object has no attribute 'lcm'
comment:18 Changed 12 years ago by
- Cc novoselt added
comment:19 Changed 12 years ago by
- Cc minz added
comment:20 follow-up: ↓ 21 Changed 12 years ago by
Has there been any work on this since sage days > 23? Even if the work is only partially finnished it would be good to know to avoid double work.
Changed 12 years ago by
Aplies to sage 4.4.4 after 1-12 patch and it also needs the #9054 patch trac_9094-sqrt-mderickx.patch
comment:21 in reply to: ↑ 20 Changed 12 years ago by
Replying to mderickx:
Has there been any work on this since sage days > 23? Even if the work is only partially finished it would be good to know to avoid double work.
There has been no further work. When I do work further on this, I will post a patch. I always post patches of everything I do as I go, as soon as I'm done with a session of work.
comment:22 Changed 12 years ago by
I am moving this entirely out of Sage and into the psage library. See
comment:23 Changed 12 years ago by
- Resolution set to wontfix
- Status changed from new to closed
I'm closing this since I'm no longer interested in getting it included in the main sage distribution. It is now in psage as mentioned above.
comment:24 Changed 12 years ago by
- Milestone changed from sage-4.6 to sage-duplicate/invalid/wontfix
comment:25 Changed 12 years ago by
- Milestone changed from sage-duplicate/invalid/wontfix to sage-wishlist
- Resolution wontfix deleted
- Status changed from closed to new
I think eventually this should be in the main sage distribution, even if no one's actively working on it right now.
comment:26 Changed 11 years ago by
comment:27 Changed 11 years ago by
- Description modified (diff)
comment:28 Changed 11 years ago by
- Cc saraedum added
comment:29 Changed 11 years ago by
The doctests of function_field.py
contain the following lines:
sage: R.<x> = FunctionField(QQ); S.<y> = R[] sage: R2.<t> = FunctionField(QQ); S2.<w> = R2[] sage: L2.<w> = R.extension((4*w)^2 - (t+1)^3 - 1)
I think it is confusing that it does not make a difference whether you write R.extension or R2.extension in this example. I'm new to sage so maybe I'm misunderstanding something here.
comment:30 Changed 11 years ago by
There are some problems with the zero of a function field:
sage: K.<x> = FunctionField(QQ); R.<y> = K[]; L.<y> = K.extension(y^2+x); sage: coerce(L,L.polynomial())==0 False sage: y/0 0
comment:31 Changed 11 years ago by
Entering the following at the sage prompt produces a TypeError: Unable to coerce -u^2 (...) to Rational
.
K.<x> = FunctionField(QQ); R.<y> = K[] L.<y> = K.extension(y^2 - x) M.<u> = FunctionField(QQ); R.<v> = M[] N.<v> = M.extension(v-u^2) L.hom([u,v])
This is due to the fact that hom()
determines the codomain by looking only at the first element of [u,v]
.
comment:32 Changed 11 years ago by
Is there a reason why a FunctionFieldMorphism is a Map and not a RingHomomorphism?
Changed 11 years ago by
Minimal support for functions field. Does not include all of the above patches.
comment:33 Changed 11 years ago by
I'm now busy with very troughly checking the entire patch wich at least with some changed free module stuff passes all doctests. There will be a big doctest patch comming up which includes tests I've thought up to also test some more none trivial examples.
There is are at least two big issues which I run in to today. They all occured in the same terminal session.
sage: K.<x> = FunctionField(QQ) sage: R.<y> = K[] sage: L.<w> = K.extension(y^5 - (x^3 + 2*x*y + 1/x)); sage: w.is_integral() False sage: L.order(w) #should raise a value error since orders can only be generated by integral elements Order in Function field in w defined by y^5 - 2*x*y + (-x^4 - 1)/x sage: L.order(w).gens() --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) /Users/maarten/Downloads/sage-4.7.2.alpha2/devel/sage-main/<ipython console> in <module>() /Users/maarten/Downloads/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/structure/parent_gens.so in sage.structure.parent_gens.ParentWithGens.gens (sage/structure/parent_gens.c:2741)() /Users/maarten/Downloads/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/structure/parent_gens.so in sage.structure.parent_gens.ParentWithGens.ngens (sage/structure/parent_gens.c:2548)() /Users/maarten/Downloads/sage-4.7.2.alpha2/local/lib/python2.6/site-packages/sage/structure/parent_gens.so in sage.structure.parent_gens.check_old_coerce (sage/structure/parent_gens.c:1228)() RuntimeError: Order in Function field in w defined by y^5 - 2*x*y + (-x^4 - 1)/x still using old coercion framework
comment:34 Changed 11 years ago by
the review patch is not entirely ready, but julian wanted to have a look so I uploaded it
comment:35 Changed 11 years ago by
At sage-devel, Maarten mentioned that pickling does not seem to work for the code posted here, which seems to be due to some attributes typically involved in coercion.
Looking at trac_9054-all-parts.patch, I see that the base class for function fields is derived from sage.rings.ring.Field
, but Field.__init__
is not called.
The rings in vanilla Sage do not pay enough attention to coercion and categories. #9944 and (in particular) #9138 aim at improving the situation. In particular, with #9138 it should now possible to avoid any direct call to ParentWithGens.__init__
; calling Field.__init__
should just work (tm). Can you try?
comment:36 Changed 11 years ago by
PS: After trac_9054-all-parts.patch was created, several other patches were posted. Can you please clearly state (in the ticket description and, for the patchbot, also in a comment) which patches are supposed to be applied? It is difficult to work on the pickling problem (or reviewing) if it is not clear what code exactly we are talking about.
comment:37 follow-ups: ↓ 38 ↓ 39 Changed 11 years ago by
Here are some comments on trac_9054-all-parts.patch:
- Please remove the
__contains__
method from the categoryFunctionFields
. Containment in categories should rely on the default implementation, unless there is a compelling reason to do otherwise.
Even worse, your containment test is ultimately based on testing class inheritance (namely in the function
is_FunctionField
). That totally undermines the category framework. It must be possible for an object to be a function field even without inheriting fromsage.rings.function_field.function_field.FunctionField
.
The default implementation of
F in FunctionFields()
relies on the category of F: The containment test returns True if and only ifF.category()
is a sub-category ofFunctionFields()
. That should be much better, from a mathematical point of view, than testing class inheritance!
- You should add a test of the form
TestSuite(F).run()
, where F is a function field. The test suite is formed by some generic tests defined in the category framework and includes many sanity tests (such as pickling for the field and its elements, associativity, commtativity, ...). If you can think of specific tests for function fields, then you should add methods named_test_...
as parent or element methods ofsage.categories.function_fields.FunctionFields
. Such methods will be automatically called when runningTestSuite(F).run()
.
- You should also add a test of the form
loads(dumps(F)) is F
, in order to test uniqueness of parent structures; if I recall correctly, the test suite from the category would only testloads(dumps(F))==F
.
- It should not be needed to have a function
is_FunctionField
(that just tests class inheritance) -F in FunctionFields()
is a better test, IMHO. If you do want to preserveis_FunctionField
then please do not simply put it in the global name space. At least, it should be deprecated, similar tois_Ring
being deprecated. There is a function decorator to do so.
- In the doc test for the
_element_constructor_
method, you explicitly call the method. I think it should better be an indirect test (after all, the documentation is supposed to show how the user is supposed to work with stuff). Hence, notL._element_constructor_(L.polynomial_ring().gen())
butL(L.polynomial_ring().gen()) #indirect doctest
.
- I already mentioned, since
FunctionField
is derived fromsage.rings.ring.Field
, thatField.__init__(...)
should be called. It could be that this only works when #9138 is used. Just callingParentWithGens.__init__
may be insufficient.
- There are several methods, such as polynomial_ring or vector_space, that use a hand-made cache. Please use the @cached_method decorator instead! That has several reasons.
- It is more easy. You don't need to manually update attributes.
- With #11115, the @cached_method decorator is rewritten in Cython and provides a faster cache than anything you could possibly create with Python.
- Is there a reason why you have a method
base_field
that simply returns the function field itself? From the behaviour of thebase_ring
method of polynomial rings, I would rather expect thatFunctionField(QQ,['t']).base_field()
returns the rational field.
comment:38 in reply to: ↑ 37 Changed 11 years ago by
Replying to SimonKing:
- I already mentioned,
... namely on sage-devel,
that
Field.__init__(...)
should be called. It could be that this only works when #9138 is used. Just callingParentWithGens.__init__
may be insufficient.
comment:39 in reply to: ↑ 37 ; follow-up: ↓ 40 Changed 11 years ago by
Replying to SimonKing:
Here are some comments on trac_9054-all-parts.patch:
- Please remove the
__contains__
method from the categoryFunctionFields
. Containment in categories should rely on the default implementation, unless there is a compelling reason to do otherwise.Even worse, your containment test is ultimately based on testing class inheritance (namely in the function
is_FunctionField
). That totally undermines the category framework. It must be possible for an object to be a function field even without inheriting fromsage.rings.function_field.function_field.FunctionField
.The default implementation of
F in FunctionFields()
relies on the category of F: The containment test returns True if and only ifF.category()
is a sub-category ofFunctionFields()
. That should be much better, from a mathematical point of view, than testing class inheritance!
Technically this is true. But this category framework instead of inheritance -- really two very different approaches to design -- leads directly to slow code in some cases in practice, which is *really* annoying, IMHO. For example, see #11657, where one of the root causes of slowness was code in is_Ring that was added to support this category approach, and which slowed everything down. Fortunately for me I have psage where I can write streamlined code without having to be weighed down, and for generic Sage working well and being extensible is more important, so of course I agree with you in this case.
- You should add a test of the form
TestSuite(F).run()
, where F is a function field. The test suite is formed by some generic tests defined in the category framework and includes many sanity tests (such as pickling for the field and its elements, associativity, commtativity, ...). If you can think of specific tests for function fields, then you should add methods named_test_...
as parent or element methods ofsage.categories.function_fields.FunctionFields
. Such methods will be automatically called when runningTestSuite(F).run()
.
- You should also add a test of the form
loads(dumps(F)) is F
, in order to test uniqueness of parent structures; if I recall correctly, the test suite from the category would only testloads(dumps(F))==F
.
This is also testing that pickling works at all. This code is used by the pickle jar to create pickles for testing later.
- It should not be needed to have a function
is_FunctionField
(that just tests class inheritance) -F in FunctionFields()
is a better test, IMHO. If you do want to preserveis_FunctionField
then please do not simply put it in the global name space. At least, it should be deprecated, similar tois_Ring
being deprecated. There is a function decorator to do so.
is_Ring is only deprecated when used from the top level (i.e., the Sage prompt). However, there is still a is_Ring function, which can be used in library code, and is not deprecated for this purpose. And the is_Ring function does test for category stuff.
- In the doc test for the
_element_constructor_
method, you explicitly call the method. I think i
t should better be an indirect test (after all, the documentation is supposed to show how the user is supposed to work with stuff). Hence, not L._element_constructor_(L.polynomial_ring().gen())
but L(L.polynomial_ring().gen()) #indirect doctest
.
I disagree. I view "#indirect test" for situations where you can't think of a clean way of directly calling the function. If there is such a way, use it! That way, at least you know for sure it is really being tested. Suggesting to get rid of that makes no sense to me. What if L(L.polynomial_ring().gen())
doesn't call _element_constructor_
at all? Also, one can also just have two tests -- one that is indirect and one that isn't.
- I already mentioned, since
FunctionField
is derived fromsage.rings.ring.Field
, thatField.__init__(...)
should be called. It could be that this only works when #9138 is used. Just callingParentWithGens.__init__
may be insufficient.
- There are several methods, such as polynomial_ring or vector_space, that use a hand-made cache. Please use the @cached_method decorator instead! That has several reasons.
- It is more easy. You don't need to manually update attributes.
- With #11115, the @cached_method decorator is rewritten in Cython and provides a faster cache than anything you could possibly create with Python.
+1. Note that when the very first version of the function field code was written (by me) @cached_method was disturbingly slow. I really, really appreciate the fast Cython rewrite.
- Is there a reason why you have a method
base_field
that simply returns the function field itself? From the behaviour of thebase_ring
method of polynomial rings, I would rather expect thatFunctionField(QQ,['t']).base_field()
returns the rational field.
No. The base field of a function field is a rational function field in 1 variable. The base field of that rational function field is then a field such as QQ. Most function fields aren't rational, e.g., they are finite extensions K/QQ(t), or even relative extensions L/K. In the first case, the base field is QQ(t) and in the second it is K. If Simon was confused by this, it should be documented better.
comment:40 in reply to: ↑ 39 ; follow-up: ↓ 41 Changed 11 years ago by
Replying to was:
Replying to SimonKing:
Here are some comments on trac_9054-all-parts.patch:
- Please remove the
__contains__
method from the categoryFunctionFields
. Containment in categories should rely on the default implementation, unless there is a compelling reason to do otherwise....
Technically this is true. But this category framework instead of inheritance -- really two very different approaches to design -- leads directly to slow code in some cases in practice, which is *really* annoying, IMHO.
A while ago, I had worked on a ticket #10667 about category containment. One purpose was to get a speedup. The trick was (again) to use Cython. For some reason, the work on that ticket has stalled. Perhaps it would be worth while to resume it.
Generally, I think it is better to improve the category framework, rather than to work around it.
For example, see #11657, where one of the root causes of slowness was code in is_Ring that was added to support this category approach, and which slowed everything down.
Then why is the existing is_Ring
not rewritten along the lines of what you do in #11657?
is_Ring is only deprecated when used from the top level (i.e., the Sage prompt).
Yes, this is what I meant. I did not mean "deprecated" in the sense of "will soon be removed", but in the sense of "please don't try this at home".
And the is_Ring function does test for category stuff.
Actually I have not been aware that category stuff is tested in is_Ring
. I was thinking about various other is_...
methods that really do nothing more than isinstance.
- Is there a reason why you have a method
base_field
that simply returns the function field itself? From the behaviour of thebase_ring
method of polynomial rings, I would rather expect thatFunctionField(QQ,['t']).base_field()
returns the rational field.No. The base field of a function field is a rational function field in 1 variable.
Ouch, so I was mistaken.
The base field of that rational function field is then a field such as QQ. Most function fields aren't rational, e.g., they are finite extensions K/QQ(t), or even relative extensions L/K. In the first case, the base field is QQ(t) and in the second it is K. If Simon was confused by this, it should be documented better.
Not needed. What I stated was based on reading the patch "diagonally". I only noticed one of the two base_field methods.
comment:41 in reply to: ↑ 40 ; follow-up: ↓ 45 Changed 11 years ago by
Replying to SimonKing:
A while ago, I had worked on a ticket #10667 about category containment. One purpose was to get a speedup. The trick was (again) to use Cython. For some reason, the work on that ticket has stalled. Perhaps it would be worth while to resume it.
+1
Generally, I think it is better to improve the category framework, rather than to work around it.
For example, see #11657, where one of the root causes of slowness was code in is_Ring that was added to support this category approach, and which slowed everything down.
Then why is the existing
is_Ring
not rewritten along the lines of what you do in #11657?
What I did there slows down is_Ring
testing if the object in question does not derive from Ring.
is_Ring is only deprecated when used from the top level (i.e., the Sage prompt).
Yes, this is what I meant. I did not mean "deprecated" in the sense of "will soon be removed", but in the sense of "please don't try this at home".
If you are developing on the Sage library, I think it is OK to use.
And the is_Ring function does test for category stuff.
Actually I have not been aware that category stuff is tested in
is_Ring
. I was thinking about various otheris_...
methods that really do nothing more than isinstance.
Yes, take a look at the code. I too was surprised by this!
-- William
comment:42 Changed 11 years ago by
- Dependencies set to #9094, #11034
- Description modified (diff)
I changed the description so that it's clear which code to look at. I will read the rest of all the remarks when I'm back from lunch.
comment:43 Changed 11 years ago by
Dear Simon,
Thanks for the help and suggestions. But sadly it did not help (altough I find #9138 a very cool ticket it's good to make a lot of rings finally more consistent with the current model of doing things with the category framework).
After some fiddeling around I managed to reduce the error to something in FunctionFieldElement_rational initialization code (hence probably not something with the categorie an coercion framework).
sage: K = QQ['x'].fraction_field(); x = K.gen(0) sage: sage.rings.function_field.function_field_element.FunctionFieldElement_rational(K, x) x sage: l=sage.rings.function_field.function_field_element.FunctionFieldElement_rational(K, x) sage: dumps(l) PicklingError Traceback (most recent call last) ... PicklingError: Can't pickle <type 'dictproxy'>: attribute lookup __builtin__.dictproxy failed sage: l.__getstate__() (Fraction Field of Univariate Polynomial Ring in x over Rational Field, <dictproxy object at 0x10ddf9948>)
comment:44 Changed 11 years ago by
It took me a while to find out how to solve the problems with pickling but I finally managed to do so. It was because of cython objects not being pickleable automatically so you have to write your own pickling methods. A more experienced programmer might have found this out way faster then me, but I had a lot of trouble (basically spent this entire afternoon reading about how pickling protocol works so I could fix it. I will now look into the issues you described and get a definite patch up.
comment:45 in reply to: ↑ 41 Changed 11 years ago by
Just for your information: I resumed work on #10667.
Testing whether QQ is a ring works faster with the methods from #11115 and #10667 than with using the current is_Ring
:
sage: C = CommutativeRings().objects() sage: QQ in C True sage: %timeit QQ in C 625 loops, best of 3: 3.88 µs per loop
versus
sage: from sage.rings.ring import is_Ring sage: %timeit is_Ring(QQ) 625 loops, best of 3: 5.06 µs per loop
Of course, just testing the class is a lot faster:
sage: from sage.rings.ring import Ring sage: %timeit isinstance(QQ,Ring) 625 loops, best of 3: 666 ns per loop
comment:46 Changed 11 years ago by
I really think that is_Ring
should be globally improved. For example, it already helps to define
def is_Ring(x): """ Return True if x is a ring. EXAMPLES:: sage: from sage.rings.ring import is_Ring sage: is_Ring(ZZ) True """ if isinstance(x, Ring): return True from sage.categories.rings import Rings return x in Rings()
hence, only do the import when needed.
The timings become
sage: from sage.rings.ring import is_Ring sage: P.<x,y,z> = QQ[] sage: is_Ring(P) True sage: %timeit is_Ring(P) 625 loops, best of 3: 243 ns per loop sage: MS = MatrixSpace(QQ,2) sage: is_Ring(MS) True sage: %timeit is_Ring(MS) 625 loops, best of 3: 21.5 µs per loop
versus
sage: from sage.rings.ring import is_Ring sage: sage: P.<x,y,z> = QQ[] sage: is_Ring(P) True sage: %timeit is_Ring(P) 625 loops, best of 3: 4.93 µs per loop sage: MS = MatrixSpace(QQ,2) sage: sage: is_Ring(MS) True sage: %timeit is_Ring(MS) 625 loops, best of 3: 26.4 µs per loop
But I think I'll move it to #10667.
comment:47 Changed 11 years ago by
Time for a little advertisement: I obtain a much improved performance with #10667 (introducing the class of objects and morphisms of a category, written in Cython). Perhaps it is useful for you?
Testing commutative rings
The function is_CommutativeRing
does nothing but testing the class. But it is a Python function. Let us compare its speed with the speed of a Cython container, testing category containment.
is_CommutativeRing
:
sage: from sage.rings.commutative_ring import is_CommutativeRing sage: is_CommutativeRing?? ... Source: def is_CommutativeRing(R): return isinstance(R, CommutativeRing) sage: is_CommutativeRing(QQ) True sage: s = SymmetricGroup(4) sage: is_CommutativeRing(s) False sage: %timeit is_CommutativeRing(QQ) 625 loops, best of 3: 1.09 µs per loop sage: %timeit is_CommutativeRing(s) 625 loops, best of 3: 3.51 µs per loop
Cython container:
sage: O = CommutativeRings().objects() sage: QQ in O True sage: s in O False sage: %timeit QQ in O 625 loops, best of 3: 1.5 µs per loop sage: %timeit s in O 625 loops, best of 3: 1.46 µs per loop
Hence, when applied to a symmetric group, the container performs a category containment test (with negative result, of course) that is faster than a Python class check!
Testing rings
As you have observed, the current is_Ring
function is suboptimal. I rewrote it in #10667.
Without #10667 (but with #11115 for a fast cache):
sage: from sage.rings.ring import is_Ring sage: MS = MatrixSpace(QQ,2) sage: %timeit is_Ring(QQ) 625 loops, best of 3: 5.1 µs per loop sage: is_Ring(MS) True sage: %timeit is_Ring(MS) 625 loops, best of 3: 17.3 µs per loop sage: C = Rings() sage: %timeit QQ in C 625 loops, best of 3: 4.18 µs per loop sage: %timeit MS in C 625 loops, best of 3: 4.31 µs per loop
With #10667 in addition:
sage: from sage.rings.ring import is_Ring sage: MS = MatrixSpace(QQ,2) sage: %timeit is_Ring(QQ) 625 loops, best of 3: 259 ns per loop sage: %timeit is_Ring(MS) 625 loops, best of 3: 17.5 µs per loop sage: C = Rings().objects() sage: %timeit QQ in C 625 loops, best of 3: 1.49 µs per loop sage: %timeit MS in C 625 loops, best of 3: 1.57 µs per loop
comment:48 Changed 11 years ago by
- Dependencies changed from #9094, #11034 to #9094, #11751
- Description modified (diff)
There seems to be an issue with returning the base ring of a RationalFunctionField?. Neither base() nor base_ring() return the correct ring: