Opened 7 years ago

Closed 6 years ago

#9054 closed enhancement (fixed)

create a class for basic function_field arithmetic for Sage

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: sage-5.0.beta2
Authors: William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff, Julian Rueth Reviewers: Maarten Derickx, Julian Rueth
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: sage-5.0.beta1 Stopgaps:

Description (last modified by jdemeyer)

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.

Note that the dependancy on #9138 is only because of a really minor change in the doctests. This ticket already has a positive review so I suspect this will get merged first. If that ticket eventually gets rejected it will be trivial to rebase the patch withouth that ticket.

Apply 9054_function_fields.patch to the Sage library.

Attachments (49)

trac_9054-part1.patch (12.5 KB) - added by was 7 years ago.
trac_9054-part2.patch (26.1 KB) - added by was 7 years ago.
trac_9054-part3.patch (23.5 KB) - added by was 7 years ago.
trac_9054-part4.patch (12.6 KB) - added by was 7 years ago.
trac_9054-part5.patch (30.3 KB) - added by robertwb 7 years ago.
trac_9054-part6.patch (9.5 KB) - added by was 7 years ago.
trac_9054-part7.patch (7.5 KB) - added by was 7 years ago.
polynomial factorization!
trac_9054-part8.patch (25.8 KB) - added by was 7 years ago.
ideals and orders!
trac_9054-part9.patch (5.7 KB) - added by was 7 years ago.
inverses of fractional ideals
trac_9054-part10.patch (10.4 KB) - added by was 7 years ago.
morphisms of function fields
trac_9054-part11.patch (12.3 KB) - added by was 7 years ago.
trac_9054-part12.patch (4.7 KB) - added by robertwb 7 years ago.
Various methods needed for #9095 (doctesets depend on #9094)
trac_9054-part1-12.patch (106.5 KB) - added by was 7 years ago.
flattened patch that incorporates all of patches 1-12 above into a single patch.
trac_9054-doctest.patch (2.8 KB) - added by mderickx 7 years ago.
Aplies to sage 4.4.4 after 1-12 patch and it also needs the #9054 patch trac_9094-sqrt-mderickx.patch
trac_9054_polynomial_base_field.patch (1.7 KB) - added by saraedum 6 years ago.
polynomial used for a field extension must be defined over the base field
trac_9054_zero.patch (2.1 KB) - added by saraedum 6 years ago.
fixes the problems regarding zero.
trac_9054_codomain.patch (1.8 KB) - added by saraedum 6 years ago.
set the correct codomain for function fields
trac_9054_doctest-2.patch (1.6 KB) - added by saraedum 6 years ago.
fixes hash doctests for 32bit and a random doctest
trac_9054_function_fields_sd32.patch (71.4 KB) - added by saraedum 6 years ago.
Minimal support for functions field. Does not include all of the above patches.
trac_9054-review.patch (39.8 KB) - added by mderickx 6 years ago.
trac_9054_undo_unittest.patch (1.5 KB) - added by saraedum 6 years ago.
revert changes to misc.unittest introduced by the review patch
trac_9054-invert_ideal.patch (1.9 KB) - added by mderickx 6 years ago.
trac_9054_isFunctionField.patch (1.4 KB) - added by saraedum 6 years ago.
use category in is_FunctionField()
trac_9054_cached_method.patch (3.8 KB) - added by saraedum 6 years ago.
replace manual caching with cached_method
trac_9054_maximal_order_member_check.patch (1.3 KB) - added by saraedum 6 years ago.
_element_constructor_ checks that element is in maximal order
trac_9054_call_super_constructors.patch (1.3 KB) - added by saraedum 6 years ago.
added missing calls to superclass constructors
trac_9054_UniqueFactory.patch (6.9 KB) - added by saraedum 6 years ago.
use UniqueFactory? instead of cached_method in constructors
trac_9054_maps_refactor.patch (14.6 KB) - added by saraedum 6 years ago.
refactored maps class hieararchy
trac_9054_doctests-3.patch (99.9 KB) - added by saraedum 6 years ago.
extended and unified doctests
trac_9054_cleanup.patch (14.6 KB) - added by saraedum 6 years ago.
cleanup code and imports
trac_9054_authors.patch (6.8 KB) - added by saraedum 6 years ago.
added authors and copyright headers
trac_9054_is_function_field.patch (1.4 KB) - added by saraedum 6 years ago.
identical to trac_9054_isFunctionField.patch but the patch bot does not like upper case in patch files
trac_9054_unique_factory.patch (6.9 KB) - added by saraedum 6 years ago.
identical to trac_9054_UniqueFactory.patch (patchbot does not like uppercase)
trac_9054_reference.patch (4.7 KB) - added by saraedum 6 years ago.
fixes in the reference manual
trac_9054_factor.patch (741 bytes) - added by saraedum 6 years ago.
fixes an import problem in factor()
trac_9054_order_category.patch (1.6 KB) - added by saraedum 6 years ago.
orders have no category set
trac_9054-all-parts.patch (122.0 KB) - added by mderickx 6 years ago.
All patches till review.patch combined
trac_9054-julian-combined.patch (132.1 KB) - added by mderickx 6 years ago.
All julians patches after review.patch combined
trac_9054-review2.patch (3.5 KB) - added by mderickx 6 years ago.
Fixes last minor errors introduced by julians patches
trac_9054_review_fixup.patch (2.0 KB) - added by saraedum 6 years ago.
patches to mderickx's review comments
trac_9054-can_this_really_be_the_last.patch (2.9 KB) - added by mderickx 6 years ago.
trac_9054_pickling.patch (1.1 KB) - added by mderickx 6 years ago.
fix pickling of FunctionField_polymod
trac_9054-all-parts.2.patch (121.8 KB) - added by saraedum 6 years ago.
provide basic function field arithmetic (combined patch by various authors)
trac_9054-julian-combined.2.patch (131.4 KB) - added by saraedum 6 years ago.
cleanup function field code and documentation
trac_9054-review2.2.patch (3.5 KB) - added by saraedum 6 years ago.
fix doctests for function fields
trac_9054_review_fixup.2.patch (2.1 KB) - added by saraedum 6 years ago.
fixes for function fields related to the review comments by mderickx
trac_9054-can_this_really_be_the_last.2.patch (2.9 KB) - added by saraedum 6 years ago.
last fixes for function fields
trac_9054_pickling.2.patch (1.1 KB) - added by saraedum 6 years ago.
fix pickling for extensions of function fields
9054_function_fields.patch (130.8 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (136)

Changed 7 years ago by was

comment:1 Changed 7 years ago by burcin

  • Cc burcin added

Changed 7 years ago by was

Changed 7 years ago by was

comment:2 follow-up: Changed 7 years ago by 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

Changed 7 years ago by was

Changed 7 years ago by robertwb

comment:3 Changed 7 years ago by robertwb

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 7 years ago by was

Changed 7 years ago by was

polynomial factorization!

comment:4 Changed 7 years ago by jen

FunctionField? constructor clips names

sage: F = FunctionField(GF(7), 'bit')
sage: F.gen()
b

Changed 7 years ago by was

ideals and orders!

Changed 7 years ago by was

inverses of fractional ideals

comment:5 in reply to: ↑ 2 Changed 7 years ago by was

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 7 years ago by was

morphisms of function fields

Changed 7 years ago by was

Changed 7 years ago by robertwb

Various methods needed for #9095 (doctesets depend on #9094)

comment:6 Changed 7 years ago by khwilson

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 7 years ago by khwilson

  • Cc khwilson added

Changed 7 years ago by was

flattened patch that incorporates all of patches 1-12 above into a single patch.

comment:8 Changed 7 years ago by was

  • 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 7 years ago by mderickx

  • Cc mderickx added

comment:10 Changed 7 years ago by mstreng

  • Cc mstreng added

comment:11 Changed 7 years ago by pbruin

  • Cc pbruin added

comment:12 Changed 7 years ago by was

  • Description modified (diff)

comment:13 Changed 7 years ago by was

NOTE: #9094 implements sqrt for polynomials, which is relevant to trac_9054-doctest.patch

comment:14 Changed 7 years ago by mderickx

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 7 years ago by was

  • Description modified (diff)

comment:16 Changed 7 years ago by mderickx

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 7 years ago by mderickx

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 7 years ago by novoselt

  • Cc novoselt added

comment:19 Changed 7 years ago by minz

  • Cc minz added

comment:20 follow-up: Changed 7 years ago by mderickx

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 7 years ago by mderickx

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 7 years ago by was

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 7 years ago by was

I am moving this entirely out of Sage and into the psage library. See

http://code.google.com/p/purplesage/issues/detail?id=3

comment:23 Changed 7 years ago by was

  • 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 7 years ago by mvngu

  • Milestone changed from sage-4.6 to sage-duplicate/invalid/wontfix

comment:25 Changed 7 years ago by robertwb

  • 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 6 years ago by minz

There should be no doctest failures left. Comments, remarks, and reviews are welcome. :)

Apply trac_9054-all-parts.patch
Depends on #9094, #11034

comment:27 Changed 6 years ago by minz

  • Authors set to William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff
  • Description modified (diff)

comment:28 Changed 6 years ago by saraedum

  • Cc saraedum added

comment:29 Changed 6 years ago by saraedum

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.

Changed 6 years ago by saraedum

polynomial used for a field extension must be defined over the base field

comment:30 Changed 6 years ago by saraedum

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

Changed 6 years ago by saraedum

fixes the problems regarding zero.

comment:31 Changed 6 years ago by saraedum

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

Changed 6 years ago by saraedum

set the correct codomain for function fields

Changed 6 years ago by saraedum

fixes hash doctests for 32bit and a random doctest

comment:32 Changed 6 years ago by saraedum

Is there a reason why a FunctionFieldMorphism is a Map and not a RingHomomorphism?

Changed 6 years ago by saraedum

Minimal support for functions field. Does not include all of the above patches.

comment:33 Changed 6 years ago by mderickx

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 6 years ago by mderickx

the review patch is not entirely ready, but julian wanted to have a look so I uploaded it

comment:35 Changed 6 years ago by SimonKing

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 6 years ago by SimonKing

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: Changed 6 years ago by SimonKing

Here are some comments on trac_9054-all-parts.patch:

  • Please remove the __contains__ method from the category FunctionFields. 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 from sage.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 if F.category() is a sub-category of FunctionFields(). 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 of sage.categories.function_fields.FunctionFields. Such methods will be automatically called when running TestSuite(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 test loads(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 preserve is_FunctionField then please do not simply put it in the global name space. At least, it should be deprecated, similar to is_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, not L._element_constructor_(L.polynomial_ring().gen()) but L(L.polynomial_ring().gen()) #indirect doctest.
  • I already mentioned, since FunctionField is derived from sage.rings.ring.Field, that Field.__init__(...) should be called. It could be that this only works when #9138 is used. Just calling ParentWithGens.__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.
    1. It is more easy. You don't need to manually update attributes.
    2. 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 the base_ring method of polynomial rings, I would rather expect that FunctionField(QQ,['t']).base_field() returns the rational field.

comment:38 in reply to: ↑ 37 Changed 6 years ago by SimonKing

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 calling ParentWithGens.__init__ may be insufficient.

comment:39 in reply to: ↑ 37 ; follow-up: Changed 6 years ago by was

Replying to SimonKing:

Here are some comments on trac_9054-all-parts.patch:

  • Please remove the __contains__ method from the category FunctionFields. 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 from sage.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 if F.category() is a sub-category of FunctionFields(). 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 of sage.categories.function_fields.FunctionFields. Such methods will be automatically called when running TestSuite(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 test loads(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 preserve is_FunctionField then please do not simply put it in the global name space. At least, it should be deprecated, similar to is_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 from sage.rings.ring.Field, that Field.__init__(...) should be called. It could be that this only works when #9138 is used. Just calling ParentWithGens.__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.
    1. It is more easy. You don't need to manually update attributes.
    2. 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 the base_ring method of polynomial rings, I would rather expect that FunctionField(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: Changed 6 years ago by SimonKing

Replying to was:

Replying to SimonKing:

Here are some comments on trac_9054-all-parts.patch:

  • Please remove the __contains__ method from the category FunctionFields. 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 the base_ring method of polynomial rings, I would rather expect that FunctionField(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: Changed 6 years ago by was

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 other is_... 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 6 years ago by mderickx

  • 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 6 years ago by mderickx

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 6 years ago by mderickx

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 6 years ago by SimonKing

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 6 years ago by SimonKing

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 6 years ago by SimonKing

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 6 years ago by mderickx

  • Dependencies changed from #9094, #11034 to #9094, #11751
  • Description modified (diff)

comment:49 Changed 6 years ago by mderickx

  • Status changed from new to needs_review

Ok I'm done with my reviewing of the original work. I guess a review patch of 39.8 KB deserves a review of its own :P

comment:50 Changed 6 years ago by mderickx

Note that this patch needs the patch at http://trac.sagemath.org/sage_trac/ticket/11751 to work, but altough the patch at that ticket makes all the doctest for function fields pass, it makes a lot of other doctests fail :(

comment:51 Changed 6 years ago by mderickx

Ok #11751 is ready for review and the code here passes all tests (at least I tested it on sage 4.7.2.alpha2 ) after you apply the tickets at 11751. So this one can finally get merged as soon as it has positive review.

Changed 6 years ago by mderickx

comment:52 Changed 6 years ago by mderickx

  • Dependencies changed from #9094, #11751 to #9094, #11751, #9138
  • Description modified (diff)

comment:53 Changed 6 years ago by mderickx

  • Description modified (diff)

Changed 6 years ago by saraedum

revert changes to misc.unittest introduced by the review patch

Changed 6 years ago by mderickx

Changed 6 years ago by saraedum

use category in is_FunctionField()

Changed 6 years ago by saraedum

replace manual caching with cached_method

Changed 6 years ago by saraedum

_element_constructor_ checks that element is in maximal order

Changed 6 years ago by saraedum

added missing calls to superclass constructors

Changed 6 years ago by saraedum

use UniqueFactory? instead of cached_method in constructors

Changed 6 years ago by saraedum

refactored maps class hieararchy

Changed 6 years ago by saraedum

extended and unified doctests

Changed 6 years ago by saraedum

cleanup code and imports

Changed 6 years ago by saraedum

added authors and copyright headers

comment:54 Changed 6 years ago by saraedum

  • Authors changed from William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff to William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff, Julian Rueth
  • Description modified (diff)
  • Milestone changed from sage-wishlist to sage-4.7.2
  • Reviewers set to Maarten Derickx, Julian Rueth

comment:55 follow-up: Changed 6 years ago by saraedum

  • Description modified (diff)

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, trac_9054_undo_unittest.patch, trac_9054-invert_ideal.patch, trac_9054_isFunctionField.patch, trac_9054_UniqueFactory.patch, trac_9054_cached_method.patch, trac_9054_maximal_order_member_check.patch, trac_9054_call_super_constructors.patch, trac_9054_maps_refactor.patch, trac_9054_doctests-3.patch, trac_9054_cleanup.patch, trac_9054_authors.patch

comment:56 Changed 6 years ago by saraedum

(Apparently the patchbot expects these "Apply" instructions in a comment and not in the ticket description)

A more detailed description of the patches since trac_9054-invert_ideal.patch:

  • trac_9054_isFunctionField.patch hopefully does what Simon King proposed for is_FunctionField
  • trac_9054_UniqueFactory.patch replaces the @cached_method in constructor.py with UniqueFactories? -- apparently that class is meant for that purpose
  • trac_9054_cached_method.patch replaces all manual caching with @cached_method methods
  • trac_9054_maximal_order_member_check.patch fixes a todo about checking that members passed to an _element_constructor are actually in the order
  • trac_9054_call_super_constructors.patch is the one I'm not sure about. At two places the super classes were not properly called -- was that by intention? I hope this fixes it.
  • trac_9054_maps_refactor.patch slightly changes the base classes of function field morphisms
  • trac_9054_doctests-3.patch essentially unifies the naming of variables in the doctests, so function fields are now called K and L, variables x, y, z. Also I added an entry to /doc/en/reference/index.rst, is that correct?
  • trac_9054_cleanup.patch reorganizes some imports and removes empty lines
  • trac_9054_authors.patch adds authors and copyrights to the files. I followed http://www.sagemath.org/doc/developer/conventions.html#headings-of-sage-library-code-files, hopefully I got it right?

I also reviewed Maarten's changes and they looked good except for the very few things I patched here. Maarten could you review my patches? It looks like a lot of work, but it should be fairly trivial to review.

Changed 6 years ago by saraedum

identical to trac_9054_isFunctionField.patch but the patch bot does not like upper case in patch files

Changed 6 years ago by saraedum

identical to trac_9054_UniqueFactory.patch (patchbot does not like uppercase)

comment:57 in reply to: ↑ 55 Changed 6 years ago by saraedum

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, trac_9054_undo_unittest.patch, trac_9054-invert_ideal.patch, trac_9054_is_function_field.patch, trac_9054_unique_factory.patch, trac_9054_cached_method.patch, trac_9054_maximal_order_member_check.patch, trac_9054_call_super_constructors.patch, trac_9054_maps_refactor.patch, trac_9054_doctests-3.patch, trac_9054_cleanup.patch, trac_9054_authors.patch

Changed 6 years ago by saraedum

fixes in the reference manual

comment:58 Changed 6 years ago by saraedum

  • Description modified (diff)

comment:59 Changed 6 years ago by saraedum

  • Status changed from needs_review to needs_work

trac_9054_cleanup.patch introduced a problem with cyclic imports ­— I'm working on it.

Changed 6 years ago by saraedum

fixes an import problem in factor()

comment:60 Changed 6 years ago by saraedum

  • Description modified (diff)

It turned out not to be a cyclic import problem but just the wrong module that was imported. I'm waiting for the doctests to set this back to needs_review.

Changed 6 years ago by saraedum

orders have no category set

comment:61 Changed 6 years ago by saraedum

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:62 Changed 6 years ago by mderickx

  • Status changed from needs_review to needs_work

On sage.math using the just released sage-4.7.2 with the following 21! patches applied

mderickx@sage:/scratch/mderickx/sage/devel/sage$ hg qser | nl
     1	9138_flat.patch
     2	trac_9054-all-parts.patch
     3	trac_9054_polynomial_base_field.patch
     4	trac_9054_zero.patch
     5	trac_9054_codomain.patch
     6	trac_9054_doctest-2.patch
     7	trac_9054-review.patch
     8	trac_9054_undo_unittest.patch
     9	trac_9054-invert_ideal.patch
    10	trac_9054_is_function_field.patch
    11	trac_9054_unique_factory.patch
    12	trac_9054_cached_method.patch
    13	trac_9054_maximal_order_member_check.patch
    14	trac_9054_call_super_constructors.patch
    15	trac_9054_maps_refactor.patch
    16	trac_9054_doctests-3.patch
    17	trac_9054_cleanup.patch
    18	trac_9054_authors.patch
    19	trac_9054_reference.patch
    20	trac_9054_factor.patch
    21	trac_9054_order_category.patch

I get

The following tests failed:

	sage -t --long devel/sage-main/sage/rings/function_field/maps.py # 1 doctests failed
	sage -t --long devel/sage-main/sage/rings/function_field/function_field.py # 7 doctests failed
----------------------------------------------------------------------
Total time for all tests: 1102.4 seconds

comment:63 Changed 6 years ago by mderickx

  • Status changed from needs_work to needs_review

Sorry false alarm. I didn't have all patches applied!

Changed 6 years ago by mderickx

All patches till review.patch combined

Changed 6 years ago by mderickx

All julians patches after review.patch combined

Changed 6 years ago by mderickx

Fixes last minor errors introduced by julians patches

comment:64 Changed 6 years ago by mderickx

  • Description modified (diff)

It turned out that also when applying all julians patches to sage 4.7.2 with #9138 we get some errors. I fixed this in my minor review2.patch. I also combined some patches so that it becomes easier for someone else to do something with this ticket (i.e. doesnt have to download 20 patches). I'm now reading trough trac_9054-julian-combined.patch if it does logical things.

comment:65 Changed 6 years ago by mderickx

  • Description modified (diff)

comment:66 Changed 6 years ago by SimonKing

Just a note on #9138: It had already been merged, but was unmerged because of an unacceptable regression in elliptic curve computations. But at #11900, I was able to avoid the regression and even turn it into a speed-up, in some cases. #11900 needs review, and then I guess #9138 would be merged again.

comment:67 follow-up: Changed 6 years ago by mderickx

  • Status changed from needs_review to needs_work

Ok these are the results from reading trough you patches:

Why did you make some_elements in function_field.py return only one element? This number should be at least two (and preferable even at least 3) since else a lot of tests in TestSuite?(F).run() will be meaningless with just one element because one element is always equal to itself for example!

If you make vector_space a cached method then why don't you change

self._vector_space = (V, from_V, to_V) 
return self._vector_space 

to

return (V, from_V, to_V) 

This code is in two places.

In function_field_order.py there is a typo in the sentence "the function field in which this iss an order."

Why did you remove:

if is_Ideal(gens): 
    gens = gens.gens() 

in function_field_order.py. I suspect the code was there to make the (not doctested) use case of:

sage: K.<x> = FunctionField(QQ) 
sage: O=K.maximal_order()
sage: I=O.ideal(x)
sage: O.ideal(I)

since you should be able to make an ideal with input an ideal.

For the rest your combination patch looks very nice. Also good that you made the documentation quality so much higher. If you either answer the above questions with the right arguments or if you change them back it seems that we can finally have function fields in sage!

comment:68 in reply to: ↑ 67 Changed 6 years ago by saraedum

Replying to mderickx:

Why did you make some_elements in function_field.py return only one element? This number should be at least two (and preferable even at least 3) since else a lot of tests in TestSuite?(F).run() will be meaningless with just one element because one element is always equal to itself for example!

I think I had seen that somewhere else only one element was returned and copied that. (at that time I didn't know what some_elements() was good for) I'll fix that.

If you make vector_space a cached method then why don't you change

self._vector_space = (V, from_V, to_V) 
return self._vector_space 

to

return (V, from_V, to_V) 

This code is in two places.

That's true. Must have missed that.

In function_field_order.py there is a typo in the sentence "the function field in which this iss an order."

Will be fixed in the next patch.

Why did you remove:

if is_Ideal(gens): 
    gens = gens.gens() 

in function_field_order.py. I suspect the code was there to make the (not doctested) use case of:

sage: K.<x> = FunctionField(QQ) 
sage: O=K.maximal_order()
sage: I=O.ideal(x)
sage: O.ideal(I)

since you should be able to make an ideal with input an ideal.

Good question. It's part of a doctest patch so I guess it just got in by accident.

For the rest your combination patch looks very nice. Also good that you made the documentation quality so much higher. If you either answer the above questions with the right arguments or if you change them back it seems that we can finally have function fields in sage!

Ok. I'll prepare a patch to fix these issues. Thanks you took the time and had a look at these patches. :)

Changed 6 years ago by saraedum

patches to mderickx's review comments

comment:69 Changed 6 years ago by saraedum

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Apply trac_9054-all-parts.patch, trac_9054-julian-combined.patch, trac_9054-review2.patch, trac_9054_review_fixup.patch.

Maarten, I'm not so sure about the is_Ideal() check anymore. Is it really expected behavior that ideal(I) creates the ideal generated by the generators of I — no matter where the ideal I lives? If you feel like that should happen, then add these two lines again and set the ticket to positive review. Or don't add them if you feel that people should be more explicit by actually calling ideal(I.gens()).

comment:70 Changed 6 years ago by mderickx

I will add it just to be consistent with numberfields.

sage: K.<a> = QQ.extension(x^2-2)
sage: I = K.ideal(3)
sage: L.<b> = K.extension(x^2-3)
sage: L.ideal(I)
Fractional ideal (3)
sage: L.ideal(p).factor()
(Fractional ideal (b))^2

Note that it also mathematically makes sense in the most general setting since the ideal created this way is the ideal extension corresponding to the coersion map from I.ring() to self.

Changed 6 years ago by mderickx

comment:71 Changed 6 years ago by mderickx

  • Description modified (diff)

comment:72 Changed 6 years ago by mderickx

If you can just check my last patch then it can have positive review.

comment:73 Changed 6 years ago by saraedum

  • Description modified (diff)
sage: K.<x> = FunctionField(QQ)
sage: R.<y> = K[]
sage: L.<y> = K.extension(y^3-x)
sage: loads(dumps(L))
AttributeError: ("'module' object has no attribute 'FunctionField_polymod'", <built-in function lookup_global>, ('FunctionField_polymod',))

This was also checked by sage: TestSuite(L).run() #long time in function_field.py.

The latest patch fixes this problem.

Maarten, if you agree with this latest patch you can set it to positive review.

comment:74 Changed 6 years ago by mderickx

I guess my last ticket name was a bit to hopefull. I just forgot to do add a --long after sage -tp 20 once and immediately a bug slips trough. I'm now testing everything with your last patch.

comment:75 Changed 6 years ago by mderickx

One more question. Shouldn't the line

FunctionField? = FunctionFieldFactory?("FunctionField?")

also be changed in a way similar in you last patch. I mean the two things should work in the same way right?

comment:76 Changed 6 years ago by saraedum

We could change it but it is not necessary. FunctionField is exported to sage.all so the pickling infrastructure can find the name there. FunctionField_polymod, however, can not be found in sage.all, that's why there is the fully qualified name.

Changed 6 years ago by mderickx

fix pickling of FunctionField_polymod

comment:77 Changed 6 years ago by mderickx

I'd like to have the consistency so I changed you last patch. If your ok with it this ticket can finally have a positive review.

comment:78 Changed 6 years ago by saraedum

  • Status changed from needs_review to positive_review

comment:79 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-pending

comment:80 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The commit messages of the patches could be cleaned up:

  1. trac_9054-julian-combined.patch: the commit message starts with * * * instead of something useful.
  2. trac_9054-review.patch has no proper commit message. This improper commit message is also in trac_9054-all-parts.patch, which should be fixed.
  3. trac_9054-all-parts.patch "contains parts 1-12, marteen's additions and final doctest fixes" makes no sense if you don't know this ticket, the message should makes sense on its own. The word "function field" does not even appear in the message of this patch!

comment:81 Changed 6 years ago by saraedum

I'm replacing the commit messages now. I don't have privileges to replace attachements so I have to upload a new set of patches instead.

Changed 6 years ago by saraedum

provide basic function field arithmetic (combined patch by various authors)

Changed 6 years ago by saraedum

cleanup function field code and documentation

Changed 6 years ago by saraedum

fix doctests for function fields

Changed 6 years ago by saraedum

fixes for function fields related to the review comments by mderickx

Changed 6 years ago by saraedum

last fixes for function fields

Changed 6 years ago by saraedum

fix pickling for extensions of function fields

comment:82 Changed 6 years ago by saraedum

  • Description modified (diff)
  • Status changed from needs_work to positive_review

Apply trac_9054-all-parts.2.patch, trac_9054-julian-combined.2.patch, trac_9054-review2.2.patch, trac_9054_review_fixup.2.patch, trac_9054-can_this_really_be_the_last.2.patch, trac_9054_pickling.2.patch

comment:83 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.0

comment:84 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Patches trac_9054-all-parts.2.patch and trac_9054-review2.2.patch apply with fuzz 2 against sage-5.0.beta1. Please rebase such that they apply cleanly.

comment:85 Changed 6 years ago by jdemeyer

  • Dependencies changed from #9094, #11751, #9138 to sage-5.0.beta1
  • Description modified (diff)
  • Status changed from needs_work to positive_review

comment:86 Changed 6 years ago by mderickx

Thanks for rebasing, I added it to my todo list, but didn't get to it yet.

Changed 6 years ago by jdemeyer

comment:87 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.0.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.