Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement from_integer and to_integer for all finite fields, extending and replacing fetch_int and integer_representation #33941

Closed
yyyyx4 opened this issue Jun 1, 2022 · 30 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Jun 1, 2022

Finite fields have .fetch_int() to decode integers into finite-field elements by reinterpreting the base-p representation as a polynomial:

sage: F.<t> = GF(7^10)
sage: el = F.fetch_int(2424); el
t^4 + 3*t + 2

However, the inverse operation is a bit cumbersome:

sage: el.polynomial().change_ring(ZZ)(el.parent().characteristic())
2424

In this patch, we add an inverse to .fetch_int(), named .to_integer() in resemblance with the Python method int.to_bytes().

For symmetry, we then rename .fetch_int() to .from_integer(): I, for one, could never remember if .fetch_int() refers to "fetching an element from an int" or "fetching this element into an int", so I think the new name makes a lot more sense.

Also, some code cleanup and optimization while we're at it.

CC: @DaveWitteMorris @slel

Component: finite rings

Author: Lorenz Panny

Branch/Commit: 0eabc46

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/33941

@yyyyx4 yyyyx4 added this to the sage-9.7 milestone Jun 1, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2022

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

4c82bferename .fetch_int() to .from_integer()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 1, 2022

Changed commit from e354108 to 4c82bfe

@yyyyx4

This comment has been minimized.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jun 1, 2022

comment:3

I just found out that #31605 already tracks this issue and that binary finite fields have .integer_representation().

This leaves us with three options for naming these methods:

  1. .fetch_int() / .integer_representation() [backwards compatible]
  2. .from_integer() / .integer_representation() [one deprecation]
  3. .from_integer() / .to_integer() [two deprecations]

Personally I'm still in favor of (3) because the others lack symmetry. Opinions?

@videlec
Copy link
Contributor

videlec commented Aug 13, 2022

comment:5

Replying to @yyyyx4:

I just found out that #31605 already tracks this issue and that binary finite fields have .integer_representation().

This leaves us with three options for naming these methods:

  1. .fetch_int() / .integer_representation() [backwards compatible]
  2. .from_integer() / .integer_representation() [one deprecation]
  3. .from_integer() / .to_integer() [two deprecations]

Personally I'm still in favor of (3) because the others lack symmetry. Opinions?

+1 to option 3

@slel
Copy link
Member

slel commented Aug 13, 2022

comment:6

Also +1 for (3) for the sake of symmetry and consistency.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 13, 2022

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

c658823Merge tag '9.7.beta6' into public/implement_inverse_of_finite_field_fetch_int
b340a29references for .integer_representation(), and use it if available as it's faster
6230ed7correct file title for NTL binary-field elements

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 13, 2022

Changed commit from 4c82bfe to 6230ed7

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 13, 2022

comment:8

Thanks for the responses! It seems the only remaining question is whether we want to deprecate .integer_representation() or not.

@slel
Copy link
Member

slel commented Aug 15, 2022

comment:9

Replying to @yyyyx4:

[...] whether [...] to deprecate .integer_representation() or not.

The only reason I would see not to would be consistency
with other software.

Do Magma, Maple, Mathematica have similar functions,
and if so how they are named?

@yyyyx4
Copy link
Member Author

yyyyx4 commented Aug 16, 2022

comment:10

I don't know. Some quick online searches didn't find anything.

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2022

Changed commit from 6230ed7 to 42d0e5f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 17, 2022

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

dd12ebcMerge tag '9.7.rc0' into public/implement_inverse_of_finite_field_fetch_int
42d0e5fMerge tag '9.8.beta2' into public/implement_inverse_of_finite_field_fetch_int

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 8, 2022

Changed commit from 42d0e5f to aeb34ec

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 8, 2022

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

aeb34ecMerge tag '9.8.beta3' into public/implement_inverse_of_finite_field_fetch_int

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 25, 2022

comment:14

Replying to Samuel Lelièvre:

Do Magma, Maple, Mathematica have similar functions,
and if so how they are named?

I could not find one for Magma and Maple. Mathematica has ToElementCode and FromElementCode

I think Sage is the leading software in this regard. No worry for consistency.

+1 to option 3.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2022

Changed commit from aeb34ec to 12411a0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2022

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

d544fe3Merge tag '9.8.beta4' into public/implement_inverse_of_finite_field_fetch_int
12411a0deprecate .fetch_int() and .integer_representation()

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 25, 2022

comment:16

Thanks.

One more thing. I think the hyphen in finite-field element is legitimate but not really necessary. The hyphen is there to mean "(finite field) element" not "finite (field element)". But sage is mathematics software and there is zero risk that one reads it as "finite (field element)". "finite field", "elliptic curve", "cyclic group", etc are all mathematical terms, and rarely written in two forms (hyphened and not hyphened). For consistency, let us stick to one form. Please reconsider.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2022

Changed commit from 12411a0 to 9e2e6ee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 25, 2022

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

9e2e6eeremove hyphens per reviewer request

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 25, 2022

comment:18

Done. (IMHO the hyphens improve readability even when they are not strictly needed to disambiguate, but it's obviously a matter of personal preference.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 26, 2022

Changed commit from 9e2e6ee to 5e92893

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 26, 2022

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

5e92893Minor edits on spaces

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 26, 2022

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

0eabc46One more edit on spaces

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 26, 2022

Changed commit from 5e92893 to 0eabc46

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 26, 2022

comment:21

Replying to Lorenz Panny:

Done. (IMHO the hyphens improve readability even when they are not strictly needed to disambiguate, but it's obviously a matter of personal preference.)

Thanks! The boundary of consistency and personal preference is blurred and unfortunately is a source of perpetual debates.

I made minor edits on spaces. Otherwise, the branch looks good to me. Set it positive review once tests pass.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 26, 2022

Reviewer: Kwankyu Lee

@yyyyx4
Copy link
Member Author

yyyyx4 commented Nov 26, 2022

comment:23

Bot looks green. Thank you!

@slel slel changed the title implement an inverse to FiniteField.fetch_int() Implement from_integer and to_integer for all finite fields, extending and replacing fetch_int and integer_representation Nov 28, 2022
@vbraun
Copy link
Member

vbraun commented Dec 11, 2022

Changed branch from public/implement_inverse_of_finite_field_fetch_int to 0eabc46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants