Opened 6 years ago

Closed 5 years ago

#17569 closed enhancement (fixed)

Allow creating finite fields without a variable name

Reported by: pbruin Owned by:
Priority: minor Milestone: sage-7.1
Component: finite rings Keywords:
Cc: ncohen, vdelecroix, jpflori, dimpase, bruno, jdemeyer, vbraun Merged in:
Authors: David Roe Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 2a92ef3 (Commits) Commit: 2a92ef3568d32310b1b63d99dc54c313f58d671e
Dependencies: Stopgaps:

Description

It should be possible to use FiniteField(p^n) (or FiniteField(p, n), see #17568) without a name argument to create the subfield of FiniteField(p).algebraic_closure() of cardinality p^n. This would mean

GF(p, n) = GF(p^n) = GF(p).algebraic_closure().subfield(n)[0]

Discussion on sage-devel: https://groups.google.com/forum/#!topic/sage-devel/LlstHp950uo

Change History (22)

comment:1 Changed 5 years ago by roed

  • Branch set to u/roed/allow_creating_finite_fields_without_a_variable_name

comment:2 Changed 5 years ago by roed

  • Authors set to David Roe
  • Commit set to a56ec7453875e5caeface30c5c7689402cf04fba
  • Milestone changed from sage-6.5 to sage-7.0
  • Status changed from new to needs_review

New commits:

a56ec74Enabling GF(p^n) with no variable name given

comment:3 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-7.0 to sage-7.1
  • Status changed from needs_review to needs_work

Why did you remove the whole # The following is a temporary solution... block? Do you want conway and prefix to continue to be supported?

  1. If no => deprecate it.
  2. If yes => document it properly (i.e. move the comment you just removed to the actual docstring) and add some doctests like
    sage: GF(3^10, conway=True, prefix='z') is GF(3^10)
    sage: GF(3^10, conway=True, prefix='foo')
    

comment:4 follow-up: Changed 5 years ago by ncohen

Hello !

I am affraid that I am not sufficiently skilled to review this branch, but I added a small commit at public/17569 which fixes a couple of typos and adds a link.

Nathann

comment:5 in reply to: ↑ 4 Changed 5 years ago by roed

Replying to ncohen:

Hello !

I am affraid that I am not sufficiently skilled to review this branch, but I added a small commit at public/17569 which fixes a couple of typos and adds a link.

Great. I have to go teach now, but I'll take a look in the next day or so.

Nathann

comment:6 Changed 5 years ago by git

  • Commit changed from a56ec7453875e5caeface30c5c7689402cf04fba to 2fcbc08667d7048d1f9a2691c8e4e9a2eff66001

Branch pushed to git repo; I updated commit sha1. New commits:

d00e732trac #17569: Typos
2fcbc08Addressing Jeroen's comments about the conway and prefix keywords for 17569.

comment:7 Changed 5 years ago by roed

  • Status changed from needs_work to needs_review

Okay, I've added some documentation about how the conway and prefix keywords are handled.

comment:8 Changed 5 years ago by jdemeyer

What's the point?

This keyword argument is now silently ignored.

Sounds like it should be deprecated...

comment:9 follow-ups: Changed 5 years ago by roed

Currently, you can feed random keyword arguments to most (all?) of the finite field constructors and they'll have no effect.

sage: GF(27,'a',blah=True)
Finite Field in a of size 3^3

I don't see a problem in having conway behave in the same manner. Why raise a deprecation warning when the code can just keep working without any change?

comment:10 in reply to: ↑ 9 Changed 5 years ago by dimpase

Replying to roed:

Currently, you can feed random keyword arguments to most (all?) of the finite field constructors and they'll have no effect.

sage: GF(27,'a',blah=True)
Finite Field in a of size 3^3

I don't see a problem in having conway behave in the same manner. Why raise a deprecation warning when the code can just keep working without any change?

You are effecting a change in the behaviour of the code. E.g. sage: GF(27,'a',conway=True) currently throws an exception, and this is will no longer be the case. (Perhaps one can come up with a more interesting example, but that's beside the point).

comment:11 in reply to: ↑ 9 Changed 5 years ago by ncohen

Replying to roed:

Currently, you can feed random keyword arguments to most (all?) of the finite field constructors and they'll have no effect.

These useless-but-accepted arguments are a plague. There was a wealth of them in many 'plot' functions at a time: you tried to give a color to something, the argument 'color="red"' was accepted but nothing happened.

The most common way in which they are a pain are typos:

sage: GF(16,'x',checkirreducible=True)
Finite Field in x of size 2^4

The side-effect of not having a '_' in check_irreducible is that... It is ignored. And of course no exception to tell you.

About having an effect, here is one (thanks to UniqueRepresentation, I expect)

sage: GF(27,'a') == GF(27,'a')
True
sage: GF(27,'a',whatever=158) == GF(27,'a')
False
sage: GF(27,'a')(1) + GF(27,'a')(1)
2
sage: GF(27,'a',whatever=158)(1) + GF(27,'a')(1)
TypeError: unsupported operand parent(s) for '+': 'Finite Field in a of size 3^3' and 'Finite Field in a of size 3^3'

Nathann

comment:12 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

Replying to roed:

Currently, you can feed random keyword arguments to most (all?) of the finite field constructors and they'll have no effect.

That's certainly a serious bug, but outside the scope of this ticket.

Still, I think that conway needs to be deprecated because it used to do something. Users should be made aware that the conway argument is no longer needed. Something like this should work:

if 'conway' in kwds:
    del kwds['conway']
    from sage.misc.superseded import deprecation
    deprecation(17569, "the 'conway' argument is deprecated, pseudo-conway polynomials are now used by default if no variable name is given") 

comment:13 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:14 Changed 5 years ago by git

  • Commit changed from 2fcbc08667d7048d1f9a2691c8e4e9a2eff66001 to 9d7c717c1afbb0d7a7abd1ad93c0c3d63043e6ad

Branch pushed to git repo; I updated commit sha1. New commits:

9d7c717Deprecating conway keyword argument to finite field constructor

comment:15 Changed 5 years ago by roed

Conflict due to #19941. Thanks Nathann.

comment:16 Changed 5 years ago by ncohen

Oh true, the rebase. Right. Give me a second.

comment:17 Changed 5 years ago by ncohen

Helloooooo !

As expected, it was a nightmare :-P

You will find the rebased branch at u/ncohen/17569.

Nathann

comment:18 Changed 5 years ago by roed

  • Branch changed from u/roed/allow_creating_finite_fields_without_a_variable_name to u/ncohen/17569
  • Commit changed from 9d7c717c1afbb0d7a7abd1ad93c0c3d63043e6ad to 69ecf50c5fc1a7ad794ad7e3e40f0493314b5d04
  • Status changed from needs_work to needs_review

Cool. Thanks for the rebase.

comment:19 Changed 5 years ago by ncohen

It was just the worst amount of 'bad'. Too bad to be 'easy', and not sufficiently bad to force me to learn how to merge two branches which work on a renamed file, in general.

Well. Next time :-P

comment:20 Changed 5 years ago by roed

  • Branch changed from u/ncohen/17569 to u/roed/17569

comment:21 Changed 5 years ago by vbraun

  • Commit changed from 69ecf50c5fc1a7ad794ad7e3e40f0493314b5d04 to 2a92ef3568d32310b1b63d99dc54c313f58d671e
  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

New commits:

2a92ef3Fixing doctest errors from conway kwd deprecation

comment:22 Changed 5 years ago by vbraun

  • Branch changed from u/roed/17569 to 2a92ef3568d32310b1b63d99dc54c313f58d671e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.