#9054: create a class for basic function_field arithmetic for Sage
---------------------------------------------------------------------------------+
Reporter: was
| Owner: was
Type: enhancement
| Status: new
Priority: major
| Milestone: sage-wishlist
Component: algebra
| Resolution:
Keywords:
| Work_issues:
Upstream: N/A
| Reviewer:
Author: William Stein, Robert Bradshaw, Maarten Derickx, Moritz Minzlaff
| Merged:
Dependencies:
|
---------------------------------------------------------------------------------+
Comment(by was):
Replying to [comment:37 SimonKing]:
> Here are some comments on [attachment: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.
>
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9054#comment:39>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
