#14990: Implement algebraic closures of finite fields
-------------------------------------+-------------------------------------
Reporter: pbruin | Owner:
Type: enhancement | Status: needs_info
Priority: major | Milestone: sage-6.2
Component: algebra | Resolution:
Keywords: finite field | Merged in:
algebraic closure | Reviewers:
Authors: Peter Bruin | Work issues:
Report Upstream: N/A | Commit:
Branch: u/pbruin/14990 | 785c7b90031db8fc32b5a271e5bca538027bcfc1
Dependencies: #14958, #13214 | Stopgaps:
-------------------------------------+-------------------------------------
Changes (by vdelecroix):
* status: needs_review => needs_info
Comment:
Hello,
Sorry, the story started already long time ago. I would be happy to finish
the review of this ticket and work again on #15390 (no trouble about the
possible rebase, I will try the cherry-pick proposed by Luca).
In case you do not want to recompile your Sage over the 6.2.rc0, I put a
version that merge the develop release at u/vdelecroix/14990 (with some
extra, see below).
I am in trouble because of the `UniqueFactory`. Firstly, in some doctests
there is a direct call to `AlgebraicClosureFiniteField_pseudo_conway` but
in practice this should be avoided as we have
{{{
sage: F = AlgebraicClosureFiniteField_pseudo_conway(GF(5),'z')
sage: z = F.an_element()
sage: z == loads(dumps(z))
False
}}}
I had no idea how to make it clear. On the other hand, what it the purpose
of a factory if there is only one implementation? Note that there will be
a problem with the generic `__reduce__` when there will be several
implementations and I am pretty sure that many others trouble will show
up.
Minor issues that I solved in a commit on my branch:
- the doctest must be in an `EXAMPLES` bloc and not an `EXAMPLE` bloc.
- I was wondering what was the point of `__getstate__` (line 473) and
`__setstate__` (line 487) in the base class. It becomes clear after
reading the code for the pseudo_conway class. Please, put some
specification (better than "used for pickling") in `__getstate__` and
`__setstate__` that would be useful to anybody who wants to implement
their own algebraic closure (all right, there should not be a lot of them,
but at least it will help people, like me, who are reading the code).
- Add optional keywords to `FiniteField.algebraic_closure` that are sent
to the factory in order to be able to do
{{{
sage: GF(5).algebraic_closure(implementation='pseudo_conway')
}}}
- for `_get_polynomial` and `_get_im_gen` we could use the decorator
`@abstract_method` (from `sage.misc.abstract_method`). That way, their
existence become part of the `TestSuite` and there is no need for the
`NotImplementedError`. There are many example of its usage in
`sage/categories`.
- It would be better to move the test lines 41-44 inside the constructor
of `AlgebraicClosure` and rather use
`AlgebraicClosureFiniteField_pseudo_conway` directly. That way we are sure
that the good class is tested.
- Implement `some_elements`: this is important for having better
automatic tests
Feel free to reuse the merge commit or the extra one from my branch. As
soon as the situation of the factory is clear to me, the branch is good to
go.
Thanks
Vincent
--
Ticket URL: <http://trac.sagemath.org/ticket/14990#comment:57>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
